Question on free() ing a hash table

[Chad]

Excercise 6-5

Write a functon undef that will remove a name and defintion from the
table maintained by lookup and install.

Code:

unsigned hash(char *s);

void undef(char *s)
{
int h;
struct nlist *prev, *np;

prev = NULL;
h = hash(s);
for(np = hashtab[h]; np!=NULL; np = np->next){
if(strcmp(s, np->name) == 0)
break;
prev = np;
}

if(np !=NULL) {
if(prev == NULL)
hashtab[h] = np->next;
else
prev->next = np->next;
free((void *) np->name);
free((void *) np->defn);
free((void *) np);
}
}

The question is, how come I have to free() both np->name and np->defn?
Ie, how come I can't just free() np?

[CBFalconer]

Chad wrote:

>

.... snip ...

>
if ((np->defn = strdup(defn)) == NULL)
return NULL;

For reasons that still elude me, every time I see this line of
code, I keep thinking that memory ISN'T allocated for np->defn.

Why? The dependency is obviously on strdup, and the return of NULL
presumably means that it failed. (strdup is non-standard.)
Therefore passing this test means it succeeded.

A better form IMO would be:

if (np->defn = strdup(defn)) return NULL;
else {
/* whatever is required */
}

and you can omit the else if you wish. But this form is clearer in
the middle of a function, while the omitted else is better used at
the start of the function.

--
Chuck F (cbfalconer at maineline dot net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home .att.net>

--
Posted via a free Usenet account from http://www.teranews.com

[Charlie Gordon]

"CBFalconer " <cb********@yah oo.coma écrit dans le message de news:
47************* **@yahoo.com...

Chad wrote:

>>

... snip ...

>>
if ((np->defn = strdup(defn)) == NULL)
return NULL;

For reasons that still elude me, every time I see this line of
code, I keep thinking that memory ISN'T allocated for np->defn.

Why? The dependency is obviously on strdup, and the return of NULL
presumably means that it failed. (strdup is non-standard.)
Therefore passing this test means it succeeded.

A better form IMO would be:

if (np->defn = strdup(defn)) return NULL;
else {
/* whatever is required */
}

Ugly style:
- assignment as a test expression: a classic cause of stupid bugs.
- if and then clauses on the same line.
- no symmetry between then and else clause.

and you can omit the else if you wish. But this form is clearer in
the middle of a function, while the omitted else is better used at
the start of the function.

Actually, direct return of NULL at the start of the function is better
without an else clause and is OK because it is easy to assert correctness.
This form would be quite error prone in the middle of a large function,
where returning NULL without proper cleanup of locally allocated memory
and/or acquired ressources would go unnoticed precisely because the return
statement does not stand out on its own. Definitely avoid this style.

--
Chqrlie.

[Peter Pichler]

santosh wrote:

In particular if 'name' and 'defn' are the only pointers that point to
their storage, then you must free() them _before_ calling free() on np,
or else you'll create a memory leak.

Your emphasis on before makes it look like the alternative is after (as
opposed to "not at all"). Doing that would be an undefined behaviour,
not just a memory leak.

[gw7rib]

On 27 Oct, 16:17, CBFalconer <cbfalco...@yah oo.comwrote:

Chad wrote:

... snip ...

if ((np->defn = strdup(defn)) == NULL)
return NULL;

For reasons that still elude me, every time I see this line of
code, I keep thinking that memory ISN'T allocated for np->defn.

Why? The dependency is obviously on strdup, and the return of NULL
presumably means that it failed. (strdup is non-standard.)
Therefore passing this test means it succeeded.

A better form IMO would be:

if (np->defn = strdup(defn)) return NULL;
else {
/* whatever is required */
}

Correct me if I'm wrong, but doesn't your version do the opposite of
the original one?