undefined | Better HN

Skip to content

Top Best Ask Show New Jobs

0 pointspeterhunt14y ago0 comments

Let me try (I do not often write production C code)

1. You need to malloc 3 words, and you're only allocating one (you want sizeof(Abc)) 2. Abc in the struct will not resolve (change to struct _Abc and the struct name to _Abc) 3. You should cast the result of malloc/check for failure 4. This is bad memory management practice, I think.

0 comments

5 comments · 2 top-level

ori_b14y ago· 3 in thread

1) Right.

2) No, 'struct Foo' is in a different namespace from the typedef'ed name. 'typedef struct Foo Foo' is perfectly valid.

3) Yes, absolutely.

4) Not sure what you mean.

Here's a hint: You'll likely miss one of them unless you realize what the algorithm is trying to do. It's an algorithmic error. The other one is findable from looking at the code without context, but the correct fix isn't entirely obvious without realizing the way the code works.

Pretty good, though :)

2arrs2ells14y ago

Abc is a node in a doubly linked list.

I think function foo is supposed to insert a new node (n) into the list before node p, but it's not done properly (while n points to p and p points to n, n does not point back to the item before p).

ori_b14y ago

I was going for foo as 'dlist_prepend()', not dlist_insert(), but figuring out that it's putting a node into a list is good enough for me. (Edit: to me, 'prepend' implies that there are no nodes before the head of the list you're prepending to)

That should be enough for you to figure the last 2 errors, in any case.

rayval14y ago

Function should return n rather than p.

gsg14y ago

Casting the result of malloc isn't a good idea in C because it can hide the lack of a prototype if you forget to #include <stdlib.h>. It's also a redundant mention of the type name in question, making for more work / noisy diffs under code change.

Unfortunately C++ requires the cast.

j / k navigate · click thread line to collapse