On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);
if (np == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don’t dispose of it yet */
}
while (false);
if (np != NULL)
{
free(np->defn);
} /*if*/
free(np);
return
result;
} /*install*/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);
if (np == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don?t dispose of it yet */
}
while (false);
if (np != NULL)
{
free(np->defn);
} /*if*/
free(np);
return
result;
} /*install*/

Both of these are truly awful.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Sun, 23 Jun 2024 02:23:43 +0300, Anton Shepelev wrote:

Why are you so afraid of `goto' ...

Look up the concept of a “Nassi-Shneiderman diagram”. It allows arbitrary nesting of complex code, with dynamic allocation going on, while
minimizing flow-control headaches. The example I posted was a simple one;
I have more complex ones if you want to see.

I think you forget to set np->name (and to free() it in
case of error).

Ah, didn’t notice that, since it was hidden in the middle of another line
of code. The fix is simple. And while I’m at it, it makes sense to factor
out the table entry disposal code into a separate routine.

void np_free(struct nlist *np)
{
if (np != NULL)
{
free(np->name);
free(np->defn);
} /*if*/
free(np);
} /*np_free*/

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);
if (np == NULL)
break;
np->name = strdup(name);
if (np->name == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don’t dispose of it yet */
}
while (false);
np_free(np);
return
result;
} /*install*/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro:

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);
if (np == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don't dispose of it yet */
}
while (false);
if (np != NULL)
{
free(np->defn);
} /*if*/
free(np);
return
result;
} /*install*/

Why are you so afraid of `goto' that you must emulate it
with `while' and `break'? I think you forget to set

name (and to free() it in case of error). You set
defn only in case of a new node, whereas the original

code did it for an existing node as well. My absolutely
untested version is below:

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{ struct nlist *np;
unsigned hashval;
int new_nm, new_nd;
void* old_fn;

new_nm = 0; new_nd = 0; old_fn = NULL;

if ((np = lookup(name)) != NULL) /* short branch first */
{ old_fn = (void *)np->defn; /* do not free it here to */
goto set_defn; } /* avoid a side effect */

np = (struct nlist *) malloc(sizeof(*np));
if(np == NULL) goto error;
new_nd = 1;

if((np->name = strdup(name)) == NULL) goto error;
new_nm = 1;

hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;

set_defn:
if ((np->defn = strdup(defn)) == NULL) goto error;

if( old_fn ) free( old_fn );
return np;
error:
if( new_nm ) free( np->name );
if( new_nd ) free( np );
return NULL;
}

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 23/06/2024 00:30, Lawrence D'Oliveiro wrote:

On Sun, 23 Jun 2024 02:23:43 +0300, Anton Shepelev wrote:

Why are you so afraid of `goto' ...

Look up the concept of a “Nassi-Shneiderman diagram”. It allows arbitrary nesting of complex code, with dynamic allocation going on, while
minimizing flow-control headaches. The example I posted was a simple one;
I have more complex ones if you want to see.

Plus of course Python doesn't have 'goto' otherwise there would be no
need of it.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-22, Lawrence D'Oliveiro <ldo@nz.invalid> wrote:

On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);

The cast is not needed in C; only if you need to compile this
as C++. The type variant of sizeof requires parentheses:

sizeof expr
sizeof (type)

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

What happening to duplicating the name into np->name?

if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don’t dispose of it yet */
}
while (false);
if (np != NULL)
{
free(np->defn);
} /*if*/
free(np);
return
result;
} /*install*/

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {
struct nlist *np = calloc(1, sizeof (struct nlist));
char *dupname = strdup(name);
char *dupdefn = strdup(defn);
unsigned hashval = hash(name);

if (np && dupname && dupdefn) {
np->name = dupname;
np->defn = dupdefn;
np->next = hashtab[hashval];
hashtab[hashval] = np;
return np;
}

free(dupdefn);
free(dupname);
free(np);

return NULL;
}
}


--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Anton Shepelev <anton.txt@gmail.moc> writes:

Lawrence D'Oliveiro:

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
break;
np = (struct nlist *)calloc(1, sizeof struct nlist);
if (np == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don't dispose of it yet */
}
while (false);
if (np != NULL)
{
free(np->defn);
} /*if*/
free(np);
return
result;
} /*install*/

Why are you so afraid of `goto' that you must emulate it
with `while' and `break'?

Why are you so fond of them that you add three extra state variables
that have to be (mentally and/or mathematically tracked) just understand
this supposedly simply function?

I think you forget to set

name (and to free() it in case of error). You set
defn only in case of a new node, whereas the original

code did it for an existing node as well. My absolutely
untested version is below:

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{ struct nlist *np;
unsigned hashval;
int new_nm, new_nd;
void* old_fn;

new_nm = 0; new_nd = 0; old_fn = NULL;

if ((np = lookup(name)) != NULL) /* short branch first */
{ old_fn = (void *)np->defn; /* do not free it here to */
goto set_defn; } /* avoid a side effect */

np = (struct nlist *) malloc(sizeof(*np));
if(np == NULL) goto error;
new_nd = 1;

if((np->name = strdup(name)) == NULL) goto error;
new_nm = 1;

hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;

set_defn:
if ((np->defn = strdup(defn)) == NULL) goto error;

if( old_fn ) free( old_fn );
return np;
error:
if( new_nm ) free( np->name );
if( new_nd ) free( np );
return NULL;
}

This, to me, is a textbook case of why goto is harmful. I have spend considerable time jumping up and down checking the state of all the
variables and I am still not sure I follow what this supposedly simple
function is doing. I am pretty sure it is not functionally the same as
the original, but it's a struggle to follow it.

(And everyone seems keen on redundant casts. Why?)

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Kaz Kylheku <643-408-1753@kylheku.com> writes:

On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

[snip attempts at tidying up...]

Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {
struct nlist *np = calloc(1, sizeof (struct nlist));
char *dupname = strdup(name);
char *dupdefn = strdup(defn);
unsigned hashval = hash(name);

if (np && dupname && dupdefn) {
np->name = dupname;
np->defn = dupdefn;
np->next = hashtab[hashval];
hashtab[hashval] = np;
return np;
}

free(dupdefn);
free(dupname);
free(np);

return NULL;
}
}

You've over-simplified. The function needs to replace the definition
with a strdup'd string (introduction another way to fail) when the name
is found by lookup. It's just another nested if that's needed. I don't
get why the goto crowd want to complicate it so much.

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Ben Bacarisse:

I don't get why the goto crowd want to complicate it so
much.

Will someone post a goto-less that fixes what I perceive as
bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

And my perception be wrong, let us first establish the
actual bug(s).

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Ben Bacarisse to Anton Shelelev:

Why are you so afraid of `goto' that you must emulate it
with `while' and `break'?

Why are you so fond of them that you add three extra state
variables that have to be (mentally and/or mathematically
tracked) just understand this supposedly simply function?

Because my attempts to fix the function without the extra
variables proved even worse.

This, to me, is a textbook case of why goto is harmful. I
have spend considerable time jumping up and down checking
the state of all the variables and I am still not sure I
follow what this supposedly simple function is doing.

I have tried to keep the structure simple: all the goto's
jump down, both the labels define isolated blocks, guared by
returns, to prevent fall-though.

(And everyone seems keen on redundant casts. Why?)

As I said, I could not test the function without extra work,
so I tried to change as little as possible.

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Kaz Kylheku:

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {

When the if-branch ends with a return, the else-branch is
redundant, and its body shall be written bare, removing one
(useless) level of nesting and indentation. This is how
goto's and multiple return's (which are but a special case
of goto) help tidy up code.

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 01:25:27 +0300, Anton Shepelev wrote:

Will someone post a goto-less that fixes what I perceive as bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

All done.

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

Not sure what the point is here.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro to Anton Shepelev:

Why are you so afraid of `goto' ...

Look up the concept of a "Nassi-Shneiderman diagram". It
allows arbitrary nesting of complex code, with dynamic
allocation going on, while minimizing flow-control
headaches.

Thank you, I will:

http://www.cs.umd.edu/hcil/members/bshneiderman/nsd/1973.pdf

I hate the traditional flowcharts, and the N.-S. certainly
look so much better.

And while I'm at it, it makes sense to factor out the
table entry disposal code into a separate routine.

void np_free(struct nlist *np)
{
if (np != NULL)
{
free(np->name);
free(np->defn);
} /*if*/
free(np);
} /*np_free*/

I thought the challenge was to fix it as a single function.
np_free() helps, but is a tad redundant in that it always
tried to dispose of the whole thing, even when at calling
point we know for certain not all three object have been
allocted. You further simplify things by zero-filling via
calloc() and relying on free() accepting NULL pointers,
whereas I prefere to avoid such redundant calls of free().

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-23, Ben Bacarisse <ben@bsb.me.uk> wrote:

Kaz Kylheku <643-408-1753@kylheku.com> writes:

On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

[snip attempts at tidying up...]

Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {
struct nlist *np = calloc(1, sizeof (struct nlist));
char *dupname = strdup(name);
char *dupdefn = strdup(defn);
unsigned hashval = hash(name);

if (np && dupname && dupdefn) {
np->name = dupname;
np->defn = dupdefn;
np->next = hashtab[hashval];
hashtab[hashval] = np;
return np;
}

free(dupdefn);
free(dupname);
free(np);

return NULL;
}
}

You've over-simplified. The function needs to replace the definition
with a strdup'd string (introduction another way to fail) when the name
is found by lookup.

I couldn't see that requirement at a glance from the way the other
code is written, but in my code I made it very clear what requirement
is being followed. All we need is to add the replacement logic
to the "existing" branch. Also, I regret not using sizeof *np:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
char *dupdefn = strdup(defn);

if (dupdefn) {
free(existing->defn);
existing->defn = dupdefn;
return existing;
}

free(dupdefn);
} else {
struct nlist *np = calloc(1, sizeof (struct nlist));
char *dupname = strdup(name);
char *dupdefn = strdup(defn);
unsigned hashval = hash(name);

if (np && dupname && dupdefn) {
np->name = dupname;
np->defn = dupdefn;
np->next = hashtab[hashval];
hashtab[hashval] = np;
return np;
}

free(dupdefn);
free(dupname);
free(np);
}

return NULL;
}

The free(dupdefn) in the first case is defensive. We know that
since there is only one resource being allocated, that's the one
that is null, so this need not be called. But if the code expands
to multiple resources, it could help remind the maintainer that
there is a cleanup block that needs to be touched.

Freeing the old definition before allocating the new one is
a mistake. Though we return null to the caller to indicate that
the ooperation failed, in doing so, we have damaged the existing
data store. There is now an entry with a null pointer definition,
that the program has to defend against.

Functions like this should try not to mutate anything existing if they
are not able to allocate the resources they need in order to succeed.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-23, Anton Shepelev <anton.txt@gmail.moc> wrote:

Kaz Kylheku:

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {

When the if-branch ends with a return, the else-branch is
redundant, and its body shall be written bare, removing one
(useless) level of nesting and indentation.

I did that because there are declarations in the else case.

I avoid mising declarations and statements, because I consider
that a language misfeature.

In a block structured language, declarations should come first,
then statements.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro to Anton Shepelev:

the failure to keep np->defn unchaged if the allocation
of the new defn value failed.

Not sure what the point is here.

The original funtion can replace the .defn member of an
existing node with NULL and terminate abnormally, corrupting
the data structure.

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse:

I don't get why the goto crowd want to complicate it so
much.

Will someone post a goto-less that fixes what I perceive as
bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

And my perception be wrong, let us first establish the
actual bug(s).

With the usual trepidation that this is untested (though I did compile
it), I'd write something like:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}
else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}
}

We have four cases:

node with the name found
new definition allocated
new definition not allocated
node with the name not found
new node, name and definition all allocated
not all of new node, name and definition allocated.

We can very simply reason about all of these situations. For example,
when is storage freed? Just before returning NULL because the name was
not in the table and one or more of the required allocations failed.
Are the calls to free legal? Yes, all are initialised with the return
from malloc-like functions and are never assigned. Pretty much anything
that you want to check can be checked by simple reasoning. In
particular, your (a), (b) and (c) can be checked quite easily.

(I've written out all the else clauses because I want to show the "fully structured" version. I admit that I might sometimes save three lines by putting "return NULL;" at the very end and allowing two cases to fall
through.)

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse to Anton Shelelev:

Why are you so afraid of `goto' that you must emulate it
with `while' and `break'?

Why are you so fond of them that you add three extra state
variables that have to be (mentally and/or mathematically
tracked) just understand this supposedly simply function?

Because my attempts to fix the function without the extra
variables proved even worse.

OK, but two labels and three extra state variables makes it much more
complex to think about. I don't think the simple "if this then that"
approach is at all problematic in this case. I can't see why adding
even one goto helps when the structured approach is so simple.

(I've tried to show this with code in another reply.)

This, to me, is a textbook case of why goto is harmful. I
have spend considerable time jumping up and down checking
the state of all the variables and I am still not sure I
follow what this supposedly simple function is doing.

I have tried to keep the structure simple: all the goto's
jump down, both the labels define isolated blocks, guared by
returns, to prevent fall-though.

Trying to make gotos less bad than they can be is not usually an overall positive. Whenever I see a "good" use of goto, I try to see if there's
a better way with no use of goto. So far (with the exception of an
example of tightly bound co-routines being simulated in a single C
function) I have not yet seen one that can't. There are sometimes
run-time considerations (giant state machines, for example) but even
there the structured code is probably clearer. The use of gotos in
those cases is not to improve the logic of the code but to placate the
code generator.

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Sun, 23 Jun 2024 00:53:04 +0100, bart wrote:

On 23/06/2024 00:30, Lawrence D'Oliveiro wrote:

On Sun, 23 Jun 2024 02:23:43 +0300, Anton Shepelev wrote:

Why are you so afraid of `goto' ...

Look up the concept of a “Nassi-Shneiderman diagram”. It allows
arbitrary nesting of complex code, with dynamic allocation going on,
while minimizing flow-control headaches.

Another point is idempotency of disposal routines. By which I mean that attempting to free a NULL pointer is a harmless no-op. How many times have
you seen pointless rigmarole like

if (p1)
free(p1);
if (p2)
free(p2);
if (p3)
free(p3);

when it could be written so much more simply as

free(p1);
free(p2);
free(p3);

You’ll notice that my np_free routine can be used in the same way.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 00:36:46 +0100, Ben Bacarisse wrote:

So far (with the exception of an example of tightly bound co-routines
being simulated in a single C function) I have not yet seen one that
can't [be done without goto].

Wouldn’t it be cool if C had continuations?

I succeeded in implementing them in a PostScript-alike toy language I’ve
been messing with (see my postings on comp.lang.postscript if you want to
know more), and they weren’t that hard to do at all.

Making proper use of them is another matter.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 01:59:40 +0300, Anton Shepelev wrote:

I thought the challenge was to fix it as a single function.

I don’t know of any “challenge”, I just saw some code that could be improved, and so I improved it.

I am pretty sure that disposal of table entries would be needed as a
separate function in a more complete lookup-table implementation, which is
why I factored it out at this point.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 00:25:56 +0100, Ben Bacarisse wrote:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}
else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}
}

We have four cases:

node with the name found
new definition allocated new definition not allocated
node with the name not found
new node, name and definition all allocated not all of new node,
name and definition allocated.

We can very simply reason about all of these situations.

Too many different paths in the control flow, though. I think it’s a good idea to minimize that.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-23, Kaz Kylheku <643-408-1753@kylheku.com> wrote:

On 2024-06-23, Anton Shepelev <anton.txt@gmail.moc> wrote:

Kaz Kylheku:

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {

When the if-branch ends with a return, the else-branch is
redundant, and its body shall be written bare, removing one
(useless) level of nesting and indentation.

I did that because there are declarations in the else case.

I avoid mising declarations and statements, because I consider
that a language misfeature.

In a block structured language, declarations should come first,
then statements.

Also:

You generally want parallel elements at the same level of indentation.

The following structure can be grating on the eyes:

if (key == node->key)
return node;
else if (key < node->key)
return tree_search(node->right, key);
return tree_search(node->left, key);

This is just an example; I realize we are not handling the case
of the key being found.

This is better:

if (key == node->key)
return node;
else if (key < node->key)
return tree_search(node->right, key);
else
return tree_search(node->left, key);

the parallel elements align at the same indentation level.

The structure being recommended by Anton is most at home in imperative situations like this:

stmt1;
stmt2;
if (condition1)
return early;
stmt3;
stmt4;
if (condition2)
return early;
stmt5;
...

When we have multiple cases that are all parallel and of equivalent
importance (they could be in in any order), not so much.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 04:38:54 +0200, Janis Papanagnou wrote:

... it is a (more or less) deeply nested imperative code
construct of loops where we want to leave from the innermost loop. Propagating the exit condition through all the nested loops "to the
surface" complicates the code here.

Maybe it’s the kind of code I write, but I typically need to do cleanups
on the way out of such nested constructs. That means a goto won’t cut it.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 24.06.2024 01:36, Ben Bacarisse wrote:

[...]

Trying to make gotos less bad than they can be is not usually an overall positive. Whenever I see a "good" use of goto, I try to see if there's
a better way with no use of goto.

This is the same impetus I have.

So far (with the exception of an
example of tightly bound co-routines being simulated in a single C
function) I have not yet seen one that can't. There are sometimes
run-time considerations (giant state machines, for example) but even
there the structured code is probably clearer. The use of gotos in
those cases is not to improve the logic of the code but to placate the
code generator.

I recall from past decades that I've seen only one specific code
pattern where I'd say that 'goto' would not make a program worse
(or even make it simpler); it is a (more or less) deeply nested
imperative code construct of loops where we want to leave from
the innermost loop. Propagating the exit condition through all
the nested loops "to the surface" complicates the code here. The
introduction of additional state or duplication of code is what
the literature (dating back to "goto considered harmful" times)
gives as (sole) rationale for its use.

It still has an inherent "bad smell" since you can circumvent a
lot code (even leaving stack contexts). Some languages have thus
introduced yet more restricted forms; e.g. the 'break N' in the
Unix standard shell language, allowing one to leave N levels of
nested (for/while/until) loops.

Janis

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 24.06.2024 01:04, Kaz Kylheku wrote:

On 2024-06-23, Anton Shepelev <anton.txt@gmail.moc> wrote:

Kaz Kylheku:

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {

When the if-branch ends with a return, the else-branch is
redundant, and its body shall be written bare, removing one
(useless) level of nesting and indentation.

I was tempted to post the same comment before I noticed that:

I did that because there are declarations in the else case.

So I at least understand where Kaz was coming from and
abstained from a comment; I think it's a valid view.

I avoid mising declarations and statements, because I consider
that a language misfeature.

In a block structured language, declarations should come first,
then statements.

This is an age old concept, some programming languages even
require that (e.g. Simula, Pascal, just to name a few). But
starting with C++ I preferred the initialized declarations;
introduce objects where you use them, so that you have sort
of a "local scope"[*].

Janis

[*] Note: By that I mean that your context is kept locally
and compact, I don't mean a block scope, or similar.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Ben Bacarisse <ben@bsb.me.uk> writes:

[...]

Trying to make gotos less bad than they can be is not usually an
overall positive. Whenever I see a "good" use of goto, I try to
see if there's a better way with no use of goto. So far [noted an
exception] I have not yet seen one that can't.

Some years ago I was revising/rewriting some code, and got to a
point where using a goto seemed like the best way to proceed.
And it was a really awful goto too, the kind of horror show one
might see in an obfuscated C contest. The argument in favor of
using the goto is that not using it would have meant a really
ugly duplication of part of the algorithm. There is no question
that I could have avoided using goto, but in that particular case
using goto seemed like a better choice than the alternatives.

Generally speaking I share your reaction to using goto.
Sometimes though using goto gives a nicer looking result
than local-scale alternatives.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 24/06/2024 02:34, Lawrence D'Oliveiro wrote:

On Mon, 24 Jun 2024 00:25:56 +0100, Ben Bacarisse wrote:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}
else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}
}

We have four cases:

node with the name found
new definition allocated new definition not allocated
node with the name not found
new node, name and definition all allocated not all of new node,
name and definition allocated.

We can very simply reason about all of these situations.

Too many different paths in the control flow, though. I think it’s a good idea to minimize that.

I disagree. It's much clearer (to me) to separate the cases and deal
with them individually.

I think the only disadvantage of doing that as a strategy is that you
can sometimes end up with duplicated code. If the duplications are significant, refactor them out as separate static functions.

Ben's code here is the first version I have seen where I have not
thought "That code is horrible. I'd have to think about it to see what
it does."

I have two small complaints. I think it is a bad idea to have more than
one variable declaration and initialisation stuffed in the same line
unless they are /very/ tightly coupled, and I certainly don't like it if pointers are involved. And I don't like returning (or using) the value
of an assignment. Basically, I prefer one line of code to do one thing
at a time. But while /I/ would split up those lines, I don't find it
hard to see what Ben is doing in the code.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-24, Janis Papanagnou <janis_papanagnou+ng@hotmail.com> wrote:

On 24.06.2024 01:04, Kaz Kylheku wrote:

On 2024-06-23, Anton Shepelev <anton.txt@gmail.moc> wrote:

Kaz Kylheku:

What scatter-brained drivel. Watch and learn:

struct nlist *install(char *name, char *defn)
{
struct nlist *existing = lookup(name);

if (existing) {
return existing;
} else {

When the if-branch ends with a return, the else-branch is
redundant, and its body shall be written bare, removing one
(useless) level of nesting and indentation.

I was tempted to post the same comment before I noticed that:

I did that because there are declarations in the else case.

So I at least understand where Kaz was coming from and
abstained from a comment; I think it's a valid view.

I avoid mising declarations and statements, because I consider
that a language misfeature.

In a block structured language, declarations should come first,
then statements.

This is an age old concept, some programming languages even
require that (e.g. Simula, Pascal, just to name a few). But
starting with C++ I preferred the initialized declarations;
introduce objects where you use them, so that you have sort
of a "local scope"[*].

But in C++ you can do this:

{
Obj foo("init", 42);
obj.barf();
}

and there are advantages to doing so, compared to leaving
the braces out.

1. you can legally goto/switch around this:

goto label; // valid C++
{
Obj foo("init", 42);
obj.barf();
}
label: ;

2. You know that the declared identifier foo's scope
ends at the curly brace:

{
Obj foo("init", 42);
obj.barf();
}

// foo is not known here; even if your editor finds a foo below
// this line, it is not *that* foo, so don't bother.

3. Because of (2) you can cleanly reuse the same name:

{
Obj foo("init", 42);
obj.barf();
}

// Copy, paste, adjust, no problem:

{
Obj foo("init", 43);
obj.barf();
}

You would never want to generate a self-contained block of code
by a macro without braces!

It is valuable to know that no references to anything called
foo outside of those braces have anything to do with that foo,
and to be able to skip around that whole thing.

Braces are encapsulation; encapsulation is often good.

Braces are a lambda that is immediately invoked!

(let ((foo 42)) <---> ((lambda (foo)
(barf foo)) (barf foo)) 42)

Variables should be initialized at the top because they are
de facto parameters of a function.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Ben Bacarisse <ben@bsb.me.uk> writes:

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse:

I don't get why the goto crowd want to complicate it so
much.

Will someone post a goto-less that fixes what I perceive as
bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

And my perception be wrong, let us first establish the
actual bug(s).

With the usual trepidation that this is untested (though I did compile
it), I'd write something like:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}
else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}
}

We have four cases:

node with the name found
new definition allocated
new definition not allocated
node with the name not found
new node, name and definition all allocated
not all of new node, name and definition allocated.

We can very simply reason about all of these situations. For example,
when is storage freed? Just before returning NULL because the name was
not in the table and one or more of the required allocations failed.
Are the calls to free legal? Yes, all are initialised with the return
from malloc-like functions and are never assigned. Pretty much anything
that you want to check can be checked by simple reasoning. In
particular, your (a), (b) and (c) can be checked quite easily.

(I've written out all the else clauses because I want to show the "fully structured" version. I admit that I might sometimes save three lines by putting "return NULL;" at the very end and allowing two cases to fall through.)

After seeing your response I decided to post my own effort. Some
names have been changed (and some const qualifiers added) but the
logical composition of the struct is the same. The main entry point
is define_or_redefine_key(). Disclaimer: compiled, not tested.


#include <stdlib.h>

typedef const char *String;
typedef struct nlist *Association;
struct nlist {
Association next;
String key;
String value;
};

extern Association lookup( String );
extern unsigned hash( String );
extern Association hashtable[];
extern char *strdup( String );

static Association find_entry( String );
static Association installed_new_entry( String );
static Association install_new( Association );
static Association new_uninstalled_association( String );
static void cfree( const void * );


Association
define_or_redefine_key( String key_string, String value_string ){
String value = strdup( value_string );
Association r = value ? find_entry( key_string ) : 0;

return
r ? cfree( r->value ), r->value = value, r
: (cfree( value ), r);
}

Association
find_entry( String key_string ){
Association entry = lookup( key_string );

return entry ? entry : installed_new_entry( key_string );
}

Association
installed_new_entry( String key_string ){
return install_new( new_uninstalled_association( key_string ) );
}

Association
install_new( Association r ){
Association *list = r ? &hashtable[ hash( r->key ) ] : 0;

return r ? r->next = *list, *list = r : r;
}

Association
new_uninstalled_association( String key_string ){
String key = strdup( key_string );
Association r = key ? malloc( sizeof *r ) : 0;

return
r ? r->next = 0, r->key = key, r->value = 0, r
: (cfree( key ), r);
}

void
cfree( const void *p ){
union { const void *pcv; void *pv; } it = (it.pcv = p, it);
free( it.pv );
}

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

On Mon, 24 Jun 2024 00:25:56 +0100, Ben Bacarisse wrote:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}
else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}
}

We have four cases:

node with the name found
new definition allocated new definition not allocated
node with the name not found
new node, name and definition all allocated not all of new node,
name and definition allocated.

We can very simply reason about all of these situations.

Too many different paths in the control flow, though. I think it’s a good idea to minimize that.

Your non-solution has more. If you fix the bug so it can change
existing entries, your code will have at yet more paths. In my code the conditions that apply to all the paths are explicit. In yours they are
the result unstructured jumps.

I suspect more than ever that you are just trolling. I don't think you
really believe your posted code is any good -- you are just throwing it
out there to provoke replies. For one thing, even after a revision it
still has a syntax error.

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Kaz Kylheku:

Also:

You generally want parallel elements at the same level of
indentation.

Who, me? As a matter of fact, /I/ do.

The following structure can be grating on the eyes:

if (key == node->key)
return node;
else if (key < node->key)
return tree_search(node->right, key);
return tree_search(node->left, key);

Yes.

This is just an example; I realize we are not handling the
case of the key being found.

This is better:

if (key == node->key)
return node;
else if (key < node->key)
return tree_search(node->right, key);
else
return tree_search(node->left, key);

Just a tiny-little-bit better, becase I the last
unconditinoal return makes sense as the default exit route.
I therefore consder the following version more expressive:

if (key == node->key) return node;
if (key < node->key) return tree_search(node->right, key);
if (1 ) return tree_search(node->left , key);

To emphasize parallelism, and line length permitting, I
format my if-else chains thus:

if (key == node->key) return node;
else if (key < node->key) return tree_search(node->right, key);
else return tree_search(node->left , key);

The structure being recommended by Anton is most at home
in imperative situations like this:

stmt1;
stmt2;
if (condition1)
return early;
stmt3;
stmt4;
if (condition2)
return early;
stmt5;
...
When we have multiple cases that are all parallel and of
equivalent importance (they could be in in any order), not
so much.

This structure is perfectly suitable for order-dependent
statements, too. And I have to use `goto' if some
deinitialisation is required before the function terminates.

--
() ascii ribbon campaign -- against html e-mail
/\ www.asciiribbon.org -- against proprietary attachments

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 11:53:51 +0100, Ben Bacarisse wrote:

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

Too many different paths in the control flow, though. I think it’s a
good idea to minimize that.

Your non-solution has more.

My solution only has one major flow of control: in the top and out the
bottom. Everything else is error checks, and it is quite obvious where
they all go--through the same cleanup path.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Mon, 24 Jun 2024 11:39:49 +0200, David Brown wrote:

It's much clearer (to me) to separate the cases and deal
with them individually.

Except it becomes difficult to ensure that you have indeed tested all
those cases.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

On Mon, 24 Jun 2024 11:53:51 +0100, Ben Bacarisse wrote:

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

Too many different paths in the control flow, though. I think it’s a
good idea to minimize that.

Your non-solution has more.

My solution only has one major flow of control: in the top and out the bottom. Everything else is error checks, and it is quite obvious where
they all go--through the same cleanup path.

Oh I see. Mine has only one major flow of control. All the others are
error checks.

You are not a serious poster.

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-24, Lawrence D'Oliveiro <ldo@nz.invalid> wrote:

On Mon, 24 Jun 2024 11:53:51 +0100, Ben Bacarisse wrote:

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

Too many different paths in the control flow, though. I think it’s a
good idea to minimize that.

Your non-solution has more.

My solution only has one major flow of control: in the top and out the bottom.

This is false. Every branch in the code creates a separate control
flow path. For instance, this has 8 possible control flow paths:

if (this)
that;

if (other)
foo;
else
bar;

if (condition)
xyzzy;

One entry at the top and one exit at the bottom does not imply
one control path.

Everything else is error checks, and it is quite obvious where
they all go--through the same cleanup path.

Your solution intertwines both cases together and jumbles the error
handling between other logic, requiring the reader to untangle all
that.

Ben's and mine solution (very similar) separates the two major
cases: name exists vs. doesn't. The former is so short,
it has no reason to share code with the other.

The happy case is consolidated together at the top of each
case: the code (1) tries to acquire all the needed resources before
doing anything, without doing error checks. Then (2) there is a single
if which checks that we have all the resources. If so, then there
is a block which configures the registration, no longer requiring
error checks. If the resource allocation failed, all the resources
are freed in one block.

The function is divided into blocks that have a single, clear
responsibility.

(If a coding convention were imposed that there must be a single exit
point from a function, the code could be easily adjusted to that without
losing these attributes.)

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 25/06/2024 01:00, Lawrence D'Oliveiro wrote:

On Mon, 24 Jun 2024 11:39:49 +0200, David Brown wrote:

It's much clearer (to me) to separate the cases and deal
with them individually.

Except it becomes difficult to ensure that you have indeed tested all
those cases.

No, it is vastly /easier/ to test the cases when they are clear and
distinct. (That doesn't necessarily mean it will be /easy/, but it will definitely be /easier/.)

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-24, Lawrence D'Oliveiro <ldo@nz.invalid> wrote:

On Mon, 24 Jun 2024 11:39:49 +0200, David Brown wrote:

It's much clearer (to me) to separate the cases and deal
with them individually.

Except it becomes difficult to ensure that you have indeed tested all
those cases.

No, it doesn't.

From an external testing point of view, it makes no difference.
External tests only see the API.

If we are looking internally at achieving coverage of key
corner cases, your code organization is definitely worse than
the nice layout into blocks that have a single responsibility.

Testing all the cases is not easy; you need something like a mocked out allocator that can be programmed to fail after N allocations.

OOM handling code is rarely well tested in real programs.

That's all the more reason why you need an absolute clear organization,
where the OOM stuff is sectioned off away from the happy case, and
you can convince yourself that it's correct by inspection.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Ben Bacarisse <ben@bsb.me.uk> writes:

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse:

I don't get why the goto crowd want to complicate it so
much.

Will someone post a goto-less that fixes what I perceive as
bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

And my perception be wrong, let us first establish the
actual bug(s).

With the usual trepidation that this is untested (though I did compile
it), I'd write something like:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}

That's fine, certainly, but I'm a fan of early fail-and-bail, which
permits the didn't-fail side of the if to remain unindented:

if (node) {
char *new_defn = strdup(defn);
if (!new_defn)
return NULL;

free(node->defn);
node->defn = new_defn;
return node;
}

else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}

I think I'd deviate a little more on this side, as I don't like doing
the strdups when you know the malloc has failed. Again, early
fail-and-bail applies:

else {
struct nlist *new_node = malloc(sizeof *new_node);
if (!new_node)
return NULL;

char *new_name = strdup(name), *new_defn = strdup(defn);
if (!new_name || !new_defn) {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}

unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}

You could split the strdups too, of course, but it's itsy-bitsy. In all honesty, I'd probably actually code with gotos, and jump to whichever
bit of cleanup was relevant. Obviously, I've not even compiled this,
we're only talking about shape of code, not generation of executables.

}

We have four cases:

node with the name found
new definition allocated
new definition not allocated
node with the name not found
new node, name and definition all allocated
not all of new node, name and definition allocated.

I once came across a quirky coding standard that would have split
this into 3 functions. The outermost if/else blocks would
have been two separate helper functions.

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node)
return update_node(node, defn);
else
return new_node(name, defn);
}

The principle being that every function should do one job, and be
visible in one small screenful. The choice of code structure in each of
the two helper functions is now simplified, as you don't need to
consider anything in the other helper function.

Phil
--
We are no longer hunters and nomads. No longer awed and frightened, as we have gained some understanding of the world in which we live. As such, we can cast aside childish remnants from the dawn of our civilization.
-- NotSanguine on SoylentNews, after Eugen Weber in /The Western Tradition/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Kaz Kylheku <643-408-1753@kylheku.com> writes:

On 2024-06-24, Lawrence D'Oliveiro <ldo@nz.invalid> wrote:

On Mon, 24 Jun 2024 11:53:51 +0100, Ben Bacarisse wrote:

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

Too many different paths in the control flow, though. I think it’s a >>>> good idea to minimize that.

Your non-solution has more.

My solution only has one major flow of control: in the top and out the
bottom.

This is false. Every branch in the code creates a separate control
flow path.

L D'O is surely just trolling. Do you think he actually considers his non-solution to be a good bit of code? Do you think he really doesn't
know what a path through some code is? He's making up terms like "major
flow of control" just to keep people talking, and I admit it's working!

Everything else is error checks, and it is quite obvious where
they all go--through the same cleanup path.

Your solution intertwines both cases together and jumbles the error
handling between other logic, requiring the reader to untangle all
that.

And it's not a solution. It has both a syntax error and a case missing (replacing an existing definition). I think he's just chucking code out
there to get a reaction. It wasn't obvious at first because old-style
trolls are so rare these days

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Phil Carmody <pc+usenet@asdf.org> writes:

Ben Bacarisse <ben@bsb.me.uk> writes:

Anton Shepelev <anton.txt@gmail.moc> writes:

Ben Bacarisse:

I don't get why the goto crowd want to complicate it so
much.

Will someone post a goto-less that fixes what I perceive as
bugs in the original:

a. the failure to free() a newly allocated nlist in case
of a later error,

b. the failure to free() a newly allocated np->name in
case of a later error,

c. the failure to keep np->defn unchaged if the
allocation of the new defn value failed.

And my perception be wrong, let us first establish the
actual bug(s).

With the usual trepidation that this is untested (though I did compile
it), I'd write something like:

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node) {
char *new_defn = strdup(defn);
if (new_defn) {
free(node->defn);
node->defn = new_defn;
return node;
}
else return NULL;
}

That's fine, certainly, but I'm a fan of early fail-and-bail, which
permits the didn't-fail side of the if to remain unindented:

if (node) {
char *new_defn = strdup(defn);
if (!new_defn)
return NULL;

free(node->defn);
node->defn = new_defn;
return node;
}

I deliberately wanted to show that the fully structured "if then else"
way was perfectly clear and simple in this trivial function. Any
deviation from the easy-to-reason about version can then be assessed to
see if it's clearer? Does the fail-and-bail here help? I don't find
one simple indented block to be at all unclear so, to my mind, no it
doesn't.

To be fair, I write a lot of code like you've shown because it's become
a habit. I think there's a lot of it out there and it's easy enough to
follow so I often do it as well. But would I change one for the other?
No.

else {
struct nlist *new_node = malloc(sizeof *new_node);
char *new_name = strdup(name), *new_defn = strdup(defn);
if (new_node && new_name && new_defn) {
unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}
else {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}
}

I think I'd deviate a little more on this side, as I don't like doing
the strdups when you know the malloc has failed.

Why? If it were gathering other key resources, like locks, then I'd
agree, but can it really hurt much?

On the plus side, it gives all the variables free-able values -- either
NULL or a free-able pointer. That simplifies reasoning about any code
that follows.

Again, early
fail-and-bail applies:

else {
struct nlist *new_node = malloc(sizeof *new_node);
if (!new_node)
return NULL;

char *new_name = strdup(name), *new_defn = strdup(defn);

But one strdup after another might have failed is OK? I see you address
this below...

if (!new_name || !new_defn) {
free(new_defn);
free(new_name);
free(new_node);
return NULL;
}

unsigned hashval = hash(name);
new_node->name = new_name;
new_node->defn = new_defn;
new_node->next = hashtab[hashval];
return hashtab[hashval] = new_node;
}

You could split the strdups too, of course, but it's itsy-bitsy.

I don't get why you do one and not the other. Either it's all
itsy-bitsy or the sequencing is better and should always be preferred.

My main objection to fail-and-bail is a strong preference for expressing
the conditions for successful operation up front.

In all
honesty, I'd probably actually code with gotos, and jump to whichever
bit of cleanup was relevant.

Why? I've yet to see a goto version of this that is even remotely as
clear as simply writing out what to do in the various cases.

We have four cases:

node with the name found
new definition allocated
new definition not allocated
node with the name not found
new node, name and definition all allocated
not all of new node, name and definition allocated.

I once came across a quirky coding standard that would have split
this into 3 functions. The outermost if/else blocks would
have been two separate helper functions.

That's not quirky; that's perfectly sound advice. I didn't do it here
because I wanted a direct comparison with all the spaghetti.

struct nlist *install(const char *name, const char *defn)
{
struct nlist *node = lookup(name);
if (node)
return update_node(node, defn);
else
return new_node(name, defn);
}

return node ? update_node(node, defn) : new_node(name, defn);

!

The principle being that every function should do one job, and be
visible in one small screenful. The choice of code structure in each of
the two helper functions is now simplified, as you don't need to
consider anything in the other helper function.

C slightly discourages this in that we need to add declarations or order
the functions "bottom-up". And, despite using static functions, it's
never clear where else the functions are being used because the smallest restriction for use of a function is the file. That might not always
matter much but this example uses a global table, so it would be
important to check that. The end result is that C programmers tend to
use fewer auxiliary functions than programmers in some other languages.

--
Ben.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Tue, 25 Jun 2024 11:47:50 +0300, Phil Carmody wrote:

In all honesty, I'd probably actually code with gotos, and jump to
whichever bit of cleanup was relevant.

Complicating the control flow, especially for cleanup paths which are
harder to test, is asking for trouble.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-26, Chris M. Thomasson <chris.m.thomasson.1@gmail.com> wrote:

On 6/25/2024 6:06 AM, Ben Bacarisse wrote:

sometimes I think its an AI...

It's an AI that curiously stopped responding to my posts early this year sometime, for no obvious REAson, other than losing arguments.

Indeed, it doesn't look ike a troll to me, though.

I think we can nickname it Miller, because it's Genuine Daft.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 26.06.2024 07:18, Kaz Kylheku wrote:

On 2024-06-26, Chris M. Thomasson <chris.m.thomasson.1@gmail.com> wrote:

sometimes I think its an AI...

This is indeed a familiar feeling when repeatedly reading stereotypical contributions from certain frequently posting bots, erm.., posters.

(In this case I think, I fear, I hope it's a human, though.)

It's an AI that curiously stopped responding to my posts early this year sometime, for no obvious REAson, other than losing arguments.

Indeed, it doesn't look ike a troll to me, though.

I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

Janis

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 2024-06-26, Janis Papanagnou <janis_papanagnou+ng@hotmail.com> wrote:

I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

"Miller Genuine Draft" is a kind of horse urine marketed as beer.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @Kazinator@mstdn.ca

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 6/26/2024 6:55 AM, Kaz Kylheku wrote:

On 2024-06-26, Janis Papanagnou <janis_papanagnou+ng@hotmail.com> wrote:

I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

"Miller Genuine Draft" is a kind of horse urine marketed as beer.

MGD is good stuff!

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 26/06/2024 12:55, Kaz Kylheku wrote:

On 2024-06-26, Janis Papanagnou <janis_papanagnou+ng@hotmail.com> wrote:

I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

"Miller Genuine Draft" is a kind of horse urine marketed as beer.

That sounds a bit country-specific for an international newsgroup. And
we should probably avoid discussing beer - people can get very worked up
about that kind of thing. It's safer to stick to less controversial and divisive topics, like politics or religion :-)

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Janis Papanagnou <janis_papanagnou+ng@hotmail.com> writes:

On 26.06.2024 07:18, Kaz Kylheku wrote:

On 2024-06-26, Chris M. Thomasson <chris.m.thomasson.1@gmail.com> wrote:

sometimes I think its an AI...

This is indeed a familiar feeling when repeatedly reading stereotypical >contributions from certain frequently posting bots, erm.., posters.

(In this case I think, I fear, I hope it's a human, though.)

It's an AI that curiously stopped responding to my posts early this year
sometime, for no obvious REAson, other than losing arguments.

Indeed, it doesn't look ike a troll to me, though.

I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

Cheap flavorless beer from Milwaukee.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Wed, 26 Jun 2024 13:24:47 +0200, David Brown wrote:

That sounds a bit country-specific for an international newsgroup. And
we should probably avoid discussing beer - people can get very worked
up about that kind of thing. It's safer to stick to less controversial
and divisive topics, like politics or religion :-)

Beer seems to resemble religion in another way: each one claims their particular one is the only true one, all the others are false.

They can’t all be right on the first point, they can all be right on the second.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 26/06/2024 12:24, David Brown wrote:

On 26/06/2024 12:55, Kaz Kylheku wrote:

On 2024-06-26, Janis Papanagnou <janis_papanagnou+ng@hotmail.com> wrote: >>>> I think we can nickname it Miller, because it's Genuine Daft.

What is that "Miller" referring to?

"Miller Genuine Draft" is a kind of horse urine marketed as beer.

That sounds a bit country-specific for an international newsgroup.  And
we should probably avoid discussing beer - people can get very worked up about that kind of thing.  It's safer to stick to less controversial and divisive topics, like politics or religion :-)

And football (but you already mentioned religion)

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 27.06.2024 01:45, Lawrence D'Oliveiro wrote:

On Wed, 26 Jun 2024 13:24:47 +0200, David Brown wrote:

That sounds a bit country-specific for an international newsgroup. And
we should probably avoid discussing beer - people can get very worked
up about that kind of thing. It's safer to stick to less controversial
and divisive topics, like politics or religion :-)

Beer seems to resemble religion in another way: each one claims their particular one is the only true one, all the others are false.

May be true in your vicinity, probably less so in Bavaria. Here there's something called "Liberalitas Bavariae". You can see that (for example)
also when sitting and relaxing in some "Biergarten". No one fights just
because of a trade mark label.

The local folks I know may have their own opinion and preferences, but
that's usually not restricted to just one sort of beer. - The situation
may be different in places where there's just two or three options for
beer available, but (as far as I have observed) not so in Bavaria.

Once folks are drunk there's a different situation, though; this is not different from other places in the world; it's a physiological matter.

Janis

They can’t all be right on the first point, they can all be right on the second.

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Fri, 21 Jun 2024 09:45:21 -0300, Thiago Adams wrote:

Page 145, The C programming Language 2 Edition

/* install: put (name, defn) in hashtab */
struct nlist *install(char *name, char *defn)
{
struct nlist *np;
unsigned hashval;

if ((np = lookup(name)) == NULL) { /* not found */
np = (struct nlist *) malloc(sizeof(*np));
if (np == NULL || (np->name = strdup(name)) == NULL)
return NULL;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
} else /* already there */
free((void *) np->defn); /* free previous defn */

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

void np_free(struct nlist *np)
{
if (np != NULL)
{
free(np->name);
free(np->defn);
} /*if*/
free(np);
} /*np_free*/

struct nlist *install(char *name, char *defn)
{
struct nlist *np = NULL;
struct nlist *result = NULL;
unsigned hashval;
do /*once*/
{
result = lookup(name);
if (result != NULL)
{
char * const defn_temp = strdup(defn);
if (defn_temp == NULL)
break;
free(result->defn);
result->defn = defn_temp;
break;
} /*if*/
np = (struct nlist *)calloc(1, sizeof (struct nlist));
if (np == NULL)
break;
np->name = strdup(name);
if (np->name == NULL)
break;
np->defn = strdup(defn);
if (np->defn == NULL)
break;
hashval = hash(name);
np->next = hashtab[hashval];
hashtab[hashval] = np;
result = np;
np = NULL; /* so I don’t dispose of it yet */
}
while (false);
np_free(np);
return
result;
} /*install*/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On Sat, 29 Jun 2024 00:02:36 -0000 (UTC), I wrote:

result = lookup(name);
if (result != NULL)
{
char * const defn_temp = strdup(defn);
if (defn_temp == NULL)
break;

Maybe replace those last two lines with

if (defn_temp == NULL)
{
result = NULL;
break;
} /*if*/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Lawrence D'Oliveiro <ldo@nz.invalid> writes:

On Tue, 25 Jun 2024 11:47:50 +0300, Phil Carmody wrote:

In all honesty, I'd probably actually code with gotos, and jump to
whichever bit of cleanup was relevant.

Complicating the control flow, especially for cleanup paths which are
harder to test, is asking for trouble.

You call it complicating, I call it simplifying.

Phil
--
We are no longer hunters and nomads. No longer awed and frightened, as we have gained some understanding of the world in which we live. As such, we can cast aside childish remnants from the dawn of our civilization.
-- NotSanguine on SoylentNews, after Eugen Weber in /The Western Tradition/

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)