Fixing a sample from K&R book using cake static analyser

[Thiago Adams]

In this video (

) I show step by step how removing warnings will fix a memory leak.


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;
}

The concepts used are

- ownership transfer
- nullable pointers

Concepts are described here

http://thradams.com/cake/ownership.html

To see the sample and play with it:

http://thradams.com/cake/playground.html

Select "find the bug" and "bug #7 K & R"

[Lawrence D'Oliveiro]

Post by Thiago Adams
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*/

[Tim Rentsch]

Post by Thiago Adams

Post by Thiago Adams
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.

[Anton Shepelev]

Post by Thiago Adams
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
np->name (and to free() it in case of error). You set
np->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;
}

[Lawrence D'Oliveiro]

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*/

[bart]

Post by Lawrence D'Oliveiro

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.

[Lawrence D'Oliveiro]

Post by Lawrence D'Oliveiro

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.

[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().

[Lawrence D'Oliveiro]

Post by Anton Shepelev
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.

[Ben Bacarisse]

Post by Anton Shepelev

Post by Thiago Adams
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?

Post by Anton Shepelev
I think you forget to set
np->name (and to free() it in case of error). You set
np->defn only in case of a new node, whereas the original
code did it for an existing node as well. My absolutely
/* 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;
if ((np->defn = strdup(defn)) == NULL) goto error;
if( old_fn ) free( old_fn );
return np;
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?)

[Anton Shepelev]

Post by Ben Bacarisse

Post by Anton Shepelev
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.

Post by Ben Bacarisse
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.

Post by Ben Bacarisse
(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.

[Ben Bacarisse]

Post by Anton Shepelev

Post by Ben Bacarisse

Post by Anton Shepelev
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.)

Post by Anton Shepelev

Post by Ben Bacarisse
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.

[Lawrence D'Oliveiro]

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.

[Janis Papanagnou]

Post by Ben Bacarisse

[...]

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.

Post by Ben Bacarisse
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

[Lawrence D'Oliveiro]

... 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.

[Tim Rentsch]

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

[...]

Post by Ben Bacarisse
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.

[Kaz Kylheku]

Post by Thiago Adams

Post by Thiago Adams
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)

Post by Thiago Adams
if (np == NULL)
break;
np->defn = strdup(defn);

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

Post by Thiago Adams
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;
}
}

[Ben Bacarisse]

Post by Thiago Adams

Post by Thiago Adams
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...]

Post by Thiago Adams
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.

[Anton Shepelev]

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).

[Lawrence D'Oliveiro]

Will someone post a goto-less that fixes what I perceive as bugs in the
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.

[Anton Shepelev]

Post by Lawrence D'Oliveiro

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.

[Lawrence D'Oliveiro]

Post by Lawrence D'Oliveiro

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.

OK, my version doesn’t do that.

[Ben Bacarisse]

Post by Anton Shepelev

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
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.)

[Lawrence D'Oliveiro]

Post by Ben Bacarisse
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;
}
}
}
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.

[David Brown]

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse
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;
}
}
}
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.

[Lawrence D'Oliveiro]

Post by David Brown
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.

[David Brown]

Post by Lawrence D'Oliveiro

Post by David Brown
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/.)

[Kaz Kylheku]

Post by Lawrence D'Oliveiro

Post by David Brown
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.

[Ben Bacarisse]

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse
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;
}
}
}
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.

[Lawrence D'Oliveiro]

Post by Ben Bacarisse

Post by Lawrence D'Oliveiro
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.

[Ben Bacarisse]

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse

Post by Lawrence D'Oliveiro
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.

[Kaz Kylheku]

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse

Post by Lawrence D'Oliveiro
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.

Post by Lawrence D'Oliveiro
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.)

[Ben Bacarisse]

Post by Kaz Kylheku

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse

Post by Lawrence D'Oliveiro
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!

Post by Kaz Kylheku

Post by Lawrence D'Oliveiro
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

[Chris M. Thomasson]

Post by Ben Bacarisse

Post by Kaz Kylheku

Post by Lawrence D'Oliveiro

Post by Ben Bacarisse

Post by Lawrence D'Oliveiro
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!

Post by Kaz Kylheku

Post by Lawrence D'Oliveiro
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

sometimes I think its an AI...

[Kaz Kylheku]

Post by Chris M. Thomasson
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.

[Janis Papanagnou]

Post by Kaz Kylheku

Post by Chris M. Thomasson
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.)

Post by Kaz Kylheku
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

[Kaz Kylheku]

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

[DFS]

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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!

[Janis Papanagnou]

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

*shudder*

Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

Janis

[Michael S]

On Wed, 26 Jun 2024 13:20:58 +0200

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

*shudder*
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)
Janis

I am not sure whether it is off topic or on topic, since it is about
advantages of not following standards very strictly.
According to my understanding, beers that *do not* follow
"Reinheitsgebot" today are very popular in Germany. I don't know
whether they are more popular than those that do follow it, or a little
less popular, but at very least they are close.
All those Weißbier of the Souths and Berliner Weisse of the North...
And that's even before we consider raising popularity of
foreign styles, esp. of IPA and stouts. IPA would be almost legal
by strict Reinheitsgebot, but only almost. Stouts can be in theory
produced in strict Reinheitsgebot manner, but I don't believe
that it is done by any modern German manufacturer.

On the other hand, mass market American lagers probably follow
"Reinheitsgebot" rather closely, Bud a little less so, Miller a little
more so. Which still does not make them decent drinks in the eyes of
pundits and even of non-pundit like myself.

[Janis Papanagnou]

Post by Michael S
On Wed, 26 Jun 2024 13:20:58 +0200

Post by Janis Papanagnou
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

I am not sure whether it is off topic or on topic, since it is about
advantages of not following standards very strictly.

Hmm.., okay. - But there's definitely no vitamin "C" in beer. :-}

Post by Michael S
According to my understanding, beers that *do not* follow
"Reinheitsgebot" today are very popular in Germany.

Frankly, I cannot tell, I have no numbers or statistical information.

WRT popularity of (specifically Bavarian) beer I hear more about the
global preference; it seems that Bavarian beer is still appreciated
worldwide - if the rumors are true and the reports based on facts not
myths or so. And rightly so. :-)

But you should know that there's many many breweries, large and small
ones in Bavaria. And every beer has it's own taste. A few are (or had
been) of relatively bad quality and were often depreciatively titled
and avoided. Generally the quality is, of course, excellent. ;-)
But since it's a matter of taste we can spare us religious wars about
what being the best; we have more than enough choices that everyone
can choose what fits best to him or her. (And we also have the mass
market for those who don't care or care less, or who just don't have
access to other sources.)

In Germany it's noteworthy to know that - again AFAICT - the most
beer is sold by few larger companies (and not in Bavaria).

WRT the Reinheitsgebot; the truth, AFAICT, is that none of the beers
we have here today conform to the (original) Reinheitsgebot any more;
there's some ingredients necessary and generally used today that don't
conform. But the Reinheitsgebot is also no formal law; it's informally
a statement in advertisement (but mostly not even mentioned any more).
What matters more are the generally valid and quite strict food laws
(and no one speaks about those as well; they are just standard).

Post by Michael S
I don't know
whether they are more popular than those that do follow it, or a little
less popular, but at very least they are close.
All those Weißbier of the Souths and Berliner Weisse of the North...
And that's even before we consider raising popularity of
foreign styles, esp. of IPA and stouts. IPA would be almost legal
by strict Reinheitsgebot, but only almost. Stouts can be in theory
produced in strict Reinheitsgebot manner, but I don't believe
that it is done by any modern German manufacturer.

There's some "foreign" (from German perspective) beer that has indeed
a market here and in other countries (Budweiser or Heinecken comes to
mind); I'm not sure whether it is because of some marketing, just hip
because it's been seen in some film, or really good. It's beer that is
also sold worldwide (by big companies). I tried some of them (and some
were okay) but given the choices I have certainly other preferences.

Post by Michael S
On the other hand, mass market American lagers probably follow
"Reinheitsgebot" rather closely, Bud a little less so, Miller a little
more so. Which still does not make them decent drinks in the eyes of
pundits and even of non-pundit like myself.

Wasn't "Bud" (if you mean the abbreviated form of "Budweiser") a beer
from the Czech Republic? (Since you mentioned American beers here?)

Hereabouts the common opinion on US American beer is not too good;
it's often - sorry guys! - disrespectfully declassified as dishwater.

As an anecdotal end; I was once inspecting the menu card of a London
pub (there was a Nethack meeting planned) and was astonished to find
an Aventinus on the card, a "heavy" dark beer from a comparably small
Bavarian brewery (Bavarian oldest Weißbier brewery). So even smaller
breweries occasionally spread.

Habe die Ehre und Prost!

Janis

[Michael S]

On Wed, 26 Jun 2024 17:32:01 +0200

Post by Janis Papanagnou

Post by Michael S
On Wed, 26 Jun 2024 13:20:58 +0200

Post by Janis Papanagnou
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

I am not sure whether it is off topic or on topic, since it is about
advantages of not following standards very strictly.

Hmm.., okay. - But there's definitely no vitamin "C" in beer. :-}

Post by Michael S
According to my understanding, beers that *do not* follow
"Reinheitsgebot" today are very popular in Germany.

Frankly, I cannot tell, I have no numbers or statistical information.
WRT popularity of (specifically Bavarian) beer I hear more about the
global preference; it seems that Bavarian beer is still appreciated
worldwide - if the rumors are true and the reports based on facts not
myths or so. And rightly so. :-)
But you should know that there's many many breweries, large and small
ones in Bavaria. And every beer has it's own taste. A few are (or had
been) of relatively bad quality and were often depreciatively titled
and avoided. Generally the quality is, of course, excellent. ;-)
But since it's a matter of taste we can spare us religious wars about
what being the best; we have more than enough choices that everyone
can choose what fits best to him or her. (And we also have the mass
market for those who don't care or care less, or who just don't have
access to other sources.)
In Germany it's noteworthy to know that - again AFAICT - the most
beer is sold by few larger companies (and not in Bavaria).
WRT the Reinheitsgebot; the truth, AFAICT, is that none of the beers
we have here today conform to the (original) Reinheitsgebot any more;
there's some ingredients necessary and generally used today that don't
conform. But the Reinheitsgebot is also no formal law; it's informally
a statement in advertisement (but mostly not even mentioned any more).
What matters more are the generally valid and quite strict food laws
(and no one speaks about those as well; they are just standard).

There are levels to non-conformance. Some times there, as you say,
some ingredients. Like minor gcc extension over C Standard.
And sometimes you have Weißbier, where they use very major component
(wheat) explicitly prohibited by Reinheitsgebot. That's no longer 'C'.
May be, it's Java. But lost of drinkers (not necessarily me) consider
it very good.

Post by Janis Papanagnou

Post by Michael S
I don't know
whether they are more popular than those that do follow it, or a
little less popular, but at very least they are close.
All those Weißbier of the Souths and Berliner Weisse of the North...
And that's even before we consider raising popularity of
foreign styles, esp. of IPA and stouts. IPA would be almost legal
by strict Reinheitsgebot, but only almost. Stouts can be in theory
produced in strict Reinheitsgebot manner, but I don't believe
that it is done by any modern German manufacturer.

There's some "foreign" (from German perspective) beer that has indeed
a market here and in other countries (Budweiser or Heinecken comes to
mind); I'm not sure whether it is because of some marketing, just hip
because it's been seen in some film, or really good. It's beer that is
also sold worldwide (by big companies). I tried some of them (and some
were okay) but given the choices I have certainly other preferences.

When I wrote "foreign-style" rather than "foreign" I meant beers
manufactured in Germany, most often by smaller manufacturer, that
resemble beers of foreign origin, most typically beers from GB,
Ireland, Belgium and craft beers from US. The exception are pilsener
beers. Despite Czech origin IMHO they can't be consider "foreign-style"
in Germany.

Post by Janis Papanagnou

Post by Michael S
On the other hand, mass market American lagers probably follow
"Reinheitsgebot" rather closely, Bud a little less so, Miller a
little more so. Which still does not make them decent drinks in the
eyes of pundits and even of non-pundit like myself.

Wasn't "Bud" (if you mean the abbreviated form of "Budweiser") a beer
from the Czech Republic? (Since you mentioned American beers here?)

I would think that when people say Bud they pretty much always mean
American mass market Budweiser beer rather then Czech beer with
similar name (Budějovický Budvar).

Post by Janis Papanagnou
Hereabouts the common opinion on US American beer is not too good;
it's often - sorry guys! - disrespectfully declassified as dishwater.
As an anecdotal end; I was once inspecting the menu card of a London
pub (there was a Nethack meeting planned) and was astonished to find
an Aventinus on the card, a "heavy" dark beer from a comparably small
Bavarian brewery (Bavarian oldest Weißbier brewery). So even smaller
breweries occasionally spread.
Habe die Ehre und Prost!
Janis

[Janis Papanagnou]

Post by Michael S
On Wed, 26 Jun 2024 17:32:01 +0200

There's some "foreign" (from German perspective) beer [...]

When I wrote "foreign-style" rather than "foreign" I meant [...]

Oh, that was coincidental; I haven't noticed at that point that
you used a similar term. I just put it in quotes because I have
some fundamental aversion how the term is often [politically]
used and instrumentalized.

Post by Michael S

Wasn't "Bud" (if you mean the abbreviated form of "Budweiser") a beer
from the Czech Republic? (Since you mentioned American beers here?)

I would think that when people say Bud they pretty much always mean
American mass market Budweiser beer rather then Czech beer with
similar name (Budějovický Budvar).

This is interesting. - Looking that up I just read about a court
case from 1907 where the US American company who was inspired by
the Czech Budweiser beer and the original European company agreed
that the US American company may call their beer Budweise only on
the North-American continent, and the Czech company in Europe.
Various name conflicts concerning that trade name were continued
until 2014 and no clear "winner" evolved, they write, and that in
the EU they consequently sell that [American] beer as "Bud".

The bottles I remember to have seen here in Germany was [1] (the
Czech one). I don't recall to have seen the US companies bottle [2]
here. (But that's just me, of course.)

Janis

[1]
Loading Image...

[2]
Loading Image...

[Scott Lurndal]

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

*shudder*
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

German bier is good, I prefer guinness or Abbots Ale myself.

[Michael S]

On Wed, 26 Jun 2024 16:26:26 GMT

Post by Scott Lurndal

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

*shudder*
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

German bier is good, I prefer guinness or Abbots Ale myself.

Do they sell Kozel Černý in California?

[Scott Lurndal]

Post by Michael S
On Wed, 26 Jun 2024 16:26:26 GMT

Post by Scott Lurndal

On 26.06.2024 12:55, Kaz Kylheku wrote: =20

wrote: =20

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

What is that "Miller" referring to? =20

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

*shudder*
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-) =20

=20
German bier is good, I prefer guinness or Abbots Ale myself.

Do they sell Kozel =C4=8Cern=C3=BD in California?

Haven't seen it, but wouldn't be surprised to find it
next to the Belgian lambics in a specialty store.

Budweiser is the most famous "Czech" beer in the US :-(

https://en.wikipedia.org/wiki/%C4%8Cesk%C3%A9_Bud%C4%9Bjovice

[Janis Papanagnou]

Post by Scott Lurndal
German bier is good, I prefer guinness or Abbots Ale myself.

I have no objection to Guinness, I just rarely see it here,
but I'm sure you can find it if you look for it. Abbots Ale
I just don't know.

Just wanted to emphasize that there's no general "German bier"
(or only as a generalization that ignores the manifold sorts).
It may fit for "German beer is [with exceptions] very good.".
(Frankly, there's also a few really awful beer products.) Tried
to outline that in a previous post that there's really many and
very different ones. (And then there's always the taste issue.)

Janis

[Chris M. Thomasson]

Post by Janis Papanagnou

Post by Scott Lurndal
German bier is good, I prefer guinness or Abbots Ale myself.

I have no objection to Guinness, I just rarely see it here,
but I'm sure you can find it if you look for it. Abbots Ale
I just don't know.
Just wanted to emphasize that there's no general "German bier"
(or only as a generalization that ignores the manifold sorts).
It may fit for "German beer is [with exceptions] very good.".
(Frankly, there's also a few really awful beer products.) Tried
to outline that in a previous post that there's really many and
very different ones. (And then there's always the taste issue.)

Easily, the worst beer on planet earth?

https://www.beeradvocate.com/beer/profile/782/51067

Humm, I still need to try this one which probably tastes a lot better:

https://www.brewdog.com/uk/tactical-nuclear-penguin

lol. ;^)

[Janis Papanagnou]

Post by Chris M. Thomasson
Easily, the worst beer on planet earth?
https://www.beeradvocate.com/beer/profile/782/51067
https://www.brewdog.com/uk/tactical-nuclear-penguin
lol. ;^)

Better than the worst? - Hard to believe! ;-)

Janis

PS: Sadly that I cannot search my favorite "worst beer on planet";
beeradvocate.com seems to require login even for a search. :-(

[Richard Harnden]

Post by Janis Papanagnou

Post by Chris M. Thomasson
Easily, the worst beer on planet earth?
https://www.beeradvocate.com/beer/profile/782/51067
https://www.brewdog.com/uk/tactical-nuclear-penguin
lol. ;^)

Better than the worst? - Hard to believe! ;-)
Janis
PS: Sadly that I cannot search my favorite "worst beer on planet";
beeradvocate.com seems to require login even for a search. :-(

Andechs, lovely stuff. https://www.andechs.de/en/monastery-brewery.html

[Janis Papanagnou]

Post by Richard Harnden
Andechs, lovely stuff. https://www.andechs.de/en/monastery-brewery.html

I wholeheartedly agree. I especially love their dark beer. :-)
Heavy stuff, though.

Janis

[Chris M. Thomasson]

Post by Janis Papanagnou

Post by Chris M. Thomasson
Easily, the worst beer on planet earth?
https://www.beeradvocate.com/beer/profile/782/51067
https://www.brewdog.com/uk/tactical-nuclear-penguin
lol. ;^)

Better than the worst? - Hard to believe! ;-)
Janis
PS: Sadly that I cannot search my favorite "worst beer on planet";
beeradvocate.com seems to require login even for a search. :-(

I am not a member of that site as well. That sucks wrt just being able
to use their search. The link was to:

Earthquake High Gravity Lager (12%)

The worst beer I have ever tasted in my entire life at 46 years old. It
was as if somebody let a bunch of apples rot; turned them in applesauce;
Mixed it with hyper cheap crap vodka; a little cheap garbage brandy; let
it set for a a day or two, then filtered the sludge. Then artificially
carbonated the filtered result; added in some more cheap vodka; canned it...

The worst beer in the world? Perhaps!

[Janis Papanagnou]

Post by Chris M. Thomasson
The worst beer I have ever tasted in my entire life at 46 years old. It
was as if somebody let a bunch of apples rot; turned them in applesauce;
Mixed it with hyper cheap crap vodka; a little cheap garbage brandy; let
it set for a a day or two, then filtered the sludge. Then artificially
carbonated the filtered result; added in some more cheap vodka; canned it...

Ranking worst on my list is something that tastes like an ashtray of
cold fag ends. It's called Rauchbier - the name stems from smoked
malt used as ingredient. Wikipedia says that these beers are nowadays
even considered a specialty.

Janis

[Kenny McCormack]

In article <v5nk05$3ivra$***@dont-email.me>,
...


I don't understand how Fixing a sample from K&R book using cake static analyser
can be off topic in this newsgroup. Sounds about as on-topic as anything
else posted here recently.

So, I don't get why the Subject line is what it is.

Now, of course, if the Subject line had been changed to:

Subject: O/T: Ramblings about alcoholic beverages

then I'd completely understand.

[Chris M. Thomasson]

Post by Kenny McCormack
...
I don't understand how Fixing a sample from K&R book using cake static analyser
can be off topic in this newsgroup. Sounds about as on-topic as anything
else posted here recently.
So, I don't get why the Subject line is what it is.
Subject: O/T: Ramblings about alcoholic beverages
then I'd completely understand.

Touche. Sorry.

[Michael S]

On Sat, 29 Jun 2024 02:20:19 +0200

Post by Janis Papanagnou

Post by Chris M. Thomasson
The worst beer I have ever tasted in my entire life at 46 years
old. It was as if somebody let a bunch of apples rot; turned them
in applesauce; Mixed it with hyper cheap crap vodka; a little cheap
garbage brandy; let it set for a a day or two, then filtered the
sludge. Then artificially carbonated the filtered result; added in
some more cheap vodka; canned it...

Ranking worst on my list is something that tastes like an ashtray of
cold fag ends. It's called Rauchbier - the name stems from smoked
malt used as ingredient. Wikipedia says that these beers are nowadays
even considered a specialty.
Janis

Similar to Islay single malts?

[Scott Lurndal]

Post by Janis Papanagnou

Post by Scott Lurndal
German bier is good, I prefer guinness or Abbots Ale myself.

I have no objection to Guinness, I just rarely see it here,
but I'm sure you can find it if you look for it. Abbots Ale
I just don't know.

Abbot Ale (I mistakenly added the s) is British; I first
was exposed to it in a pub near Milton Keynes.

Post by Janis Papanagnou
Just wanted to emphasize that there's no general "German bier"

Oh, indeed. The biggies stateside for European beers are Becks,
Stella Artois and Heineken. Although one local restaurant has
Spaten on draft. I like the Spaten octoberfest.

Post by Janis Papanagnou
(or only as a generalization that ignores the manifold sorts).
It may fit for "German beer is [with exceptions] very good.".

Unfortunately, last time I was actually in Munchen I was
a couple years short of drinking age.

Post by Janis Papanagnou
(Frankly, there's also a few really awful beer products.)

Ah, Pabst Blue Ribbon and Grainbelt.

[Janis Papanagnou]

Post by Scott Lurndal
Unfortunately, last time I was actually in Munchen I was
a couple years short of drinking age.

You know that in Munich (as in all of Bavaria) you may -
very *unofficially*!!! - start drinking beer earlier than
elsewhere. (Though in supermarkets they must make an age
check nowadays.) But otherwise no one seems to really care.

I suppose that nowadays (with the global market) you can get
almost anything you want also in other countries. In several
towns (also in Munich) there was or still is a "House of the
111 (or so) Beers", where (despite the abundance of existing
German beers) you could also get a lot of beers from other
countries. (I think that was also where I've quaffed my first
Guinness.)

There's a "philosophy" (sort of - some call it differently)
that beer counts not as alcohol but as "liquid bread". So
excessive consume like on the Oktoberfest is better endured.

Post by Scott Lurndal

Post by Janis Papanagnou
(Frankly, there's also a few really awful beer products.)

Ah, Pabst Blue Ribbon and Grainbelt.

I don't know these. - And of course I have my own list. ;-)

Janis

[Phil Carmody]

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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.

*shudder*
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

The best thing about the Reinheitsgebot is that it specifies that
half of the year a Mass of bier may not exceed one Pfennig in price,
and for the other half of the year it may not exceed two Pfennigs.
And you say german brewers are still abiding by that, do you? If
so, I must visit.

http://www.europeanbeerguide.net/reinheit.htm
"""
The German Reinheitsgebot - why it's a load of old bollocks

Introduction

The Reinheitsgebot, the oldest consumer protection law and a guarantee
of beer quality. An example to the world of how beer should be brewed,
as the Germans have done for centuries. Well, not really. These are a
few of the myths I would like to expose. Everyone thinks that they know
what the Reinheitsgebot is and mostly consider that's it's pretty
groovy. This is an attempt to have an objective look at what can be a
very emotive subject.

Now, some people may be a little shocked and perhaps even outraged by
the title of this page so a few words of explanation first. German beer,
generally, is brewed to a very high standard, one which of the rest of
the world rightly envies. Unfortunately, many people seem to get
confused about the reasons for the high quality of German beer. As far
as I can tell, the Reinheitsgebot is totally irrelevant; German beer is
good because German brewers are highly skilled and make their beer with
pride and care.
"""

Phil

[Janis Papanagnou]

Post by Phil Carmody

Post by Janis Papanagnou
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

The best thing about the Reinheitsgebot is that it specifies that
half of the year a Mass of bier may not exceed one Pfennig in price,
and for the other half of the year it may not exceed two Pfennigs.

(That's just one part of the text, the other part is as indicated
by its name Reinheitsgebot (~ "purity commandment") about quality
by specifying the allowed ingredients. It was a counter measure
against fraud and profiteering.)

Post by Phil Carmody
And you say german brewers are still abiding by that, do you? If
so, I must visit.

No. If you want cheep beer stay away. The beer prices today are
horribly high. For example on the Oktoberfest one Mass (1 liter)
had cost about 14.50 Euro last year (IIRC). In a nice Biergarten
in Munich you still pay something ranging from about 8.50 Euro
to 11.00 Euro.

Frankly, if I'd see some beer offered for one or two Pfennig I'd
be very reluctant to even taste that. ;-)

Janis

[Michael S]

On Fri, 28 Jun 2024 14:04:01 +0200

Post by Janis Papanagnou

Post by Phil Carmody

Post by Janis Papanagnou
Glad we have here, where I live, something called "Reinheitsgebot"
[https://en.wikipedia.org/wiki/Reinheitsgebot] and also a history
of quality beer, of course. :-)

The best thing about the Reinheitsgebot is that it specifies that
half of the year a Mass of bier may not exceed one Pfennig in price,
and for the other half of the year it may not exceed two Pfennigs.

(That's just one part of the text, the other part is as indicated
by its name Reinheitsgebot (~ "purity commandment") about quality
by specifying the allowed ingredients. It was a counter measure
against fraud and profiteering.)

Post by Phil Carmody
And you say german brewers are still abiding by that, do you? If
so, I must visit.

No. If you want cheep beer stay away. The beer prices today are
horribly high. For example on the Oktoberfest one Mass (1 liter)
had cost about 14.50 Euro last year (IIRC). In a nice Biergarten
in Munich you still pay something ranging from about 8.50 Euro
to 11.00 Euro.
Frankly, if I'd see some beer offered for one or two Pfennig I'd
be very reluctant to even taste that. ;-)
Janis

On other forum I was told by people living in Germany that in
supermarkets they can get 500ml can of drinkable liquid for 0.4E.
Of course, it's not a Martzen lager they drink during festivals, and
not Andechs Doppelbock Dunkel (which I pesonally likely wouldn't like
because of too high ABV), but still it is both better and cheaper than
the cheapest beers in majority of the Western countries.

[Janis Papanagnou]

Post by Michael S
On other forum I was told by people living in Germany that in
supermarkets they can get 500ml can of drinkable liquid for 0.4E.
Of course, it's not a Martzen lager they drink during festivals, and
not Andechs Doppelbock Dunkel (which I pesonally likely wouldn't like
because of too high ABV), but still it is both better and cheaper than
the cheapest beers in majority of the Western countries.

I cannot tell about other countries; only in Scandinavia I personally
experienced generally very high prices for alcoholics, also in shops).
Beer in Greece (for example) wasn't that expensive, as far as I recall,
closer to German prices.

In Germany you'll get, of course, much better prices in Supermarkets
(if compared to the prices in a Biergarten, pubs, taverns, and events
like Oktoberfest).

The 40 ct for a standard (0.5 l) bottle is certainly at the low end
of the scale; the one I (only occasionally) buy is 1+ Euro. And in
supermarkets (or liquor shops) you also get Andechser beer also for
a much better price than in the monastery's Biergarten or tavern; you
pay not only for the beer but also for service, atmosphere, etc., and
as long as there's demand the market defines these horrible prices.

My last Biergarten beer (this week) was 5.60 Euro (for 0.5 l). In a
restaurant 4.60 Euro. (Just some typical values.)

Janis

[Tim Rentsch]

Post by Michael S
On other forum I was told by people living in Germany that in
supermarkets they can get 500ml can of drinkable liquid for 0.4E.
Of course, it's not a Martzen lager they drink during festivals, and
not Andechs Doppelbock Dunkel (which I pesonally likely wouldn't like
because of too high ABV), but still it is both better and cheaper than
the cheapest beers in majority of the Western countries.

I cannot tell about other countries; only in Scandinavia I personally
experienced generally very high prices for alcoholics, also in shops).
Beer in Greece (for example) wasn't that expensive, as far as I recall,
closer to German prices. [...]

It would be nice if the discussion of different beers could be
taken to a newsgroup where it is more topical, as for example
to comp.lang.c++, where some amount of alcohol seems to be
necessary to be able to endure the language.

[Janis Papanagnou]

Post by Tim Rentsch
It would be nice if the discussion of different beers could be
taken to a newsgroup where it is more topical, as for example
to comp.lang.c++, where some amount of alcohol seems to be
necessary to be able to endure the language.

Serious(!) suggestions are (would be) always welcome. (At least
I spot some humor here, which is a Good Thing. :-)

(Otherwise, just a suggestion, try to ignore [OT] marked posts.)

More on-topic; I'm curious what problems a C professional has
with C++. Myself the what annoys me most about C++ was its C
base... - well, and the newer evolutions (C++11 standard, e.g.);
but alcohol doesn't help here. ;-)

Janis

[Tim Rentsch]

Post by Tim Rentsch
It would be nice if the discussion of different beers could be
taken to a newsgroup where it is more topical, as for example
to comp.lang.c++, where some amount of alcohol seems to be
necessary to be able to endure the language.

Serious(!) suggestions are (would be) always welcome. (At least
I spot some humor here, which is a Good Thing. :-)

My request that the discussion of different beers be taken to a
different newsgroup was a serious suggestion, even if the part
about comp.lang.c++ was not.

(Otherwise, just a suggestion, try to ignore [OT] marked posts.)

Your suggestion has approximately the same content as one to try
to ignore assholes. In other words it's roughly equivalent to
saying Fuck you.

More on-topic; I'm curious what problems a C professional has
with C++.

Such a discussion is better carried out in comp.lang.c++ than here,
both because it is more related to C++ than it is to C, and also
because there is a greater level of expertise in C++ there than in
comp.lang.c. I expect that essentially all C++ developers have a
pretty good working knowledge of C, whereas many C developers do
not have a good working knowledge of C++.

[Chris M. Thomasson]

Post by Janis Papanagnou

Post by Michael S
On other forum I was told by people living in Germany that in
supermarkets they can get 500ml can of drinkable liquid for 0.4E.
Of course, it's not a Martzen lager they drink during festivals, and
not Andechs Doppelbock Dunkel (which I pesonally likely wouldn't like
because of too high ABV), but still it is both better and cheaper than
the cheapest beers in majority of the Western countries.

I cannot tell about other countries; only in Scandinavia I personally
experienced generally very high prices for alcoholics, also in shops).
Beer in Greece (for example) wasn't that expensive, as far as I recall,
closer to German prices.

[...]

Shit, I payed $16 for two shots of well whiskey at one of the big clubs
in Reno. It kind of pissed me off because I can buy a cheap bottle of
not so terrible booze for around $20. They are making a killing (bars)
off of selling drinks. Just put a dollar in a slot machine and spend one
cent per 15 minutes. When the drink girl comes by, ask for a good beer.
Then tip her say, $5+ dollars. She might like that, and come back... You
make sure to bet every 15 minutes because you need to be actively
gambling...

[Chris M. Thomasson]

[...]

Post by Chris M. Thomasson
Shit, I payed $16 for two shots of well whiskey at one of the big clubs
in Reno.

I wonder how much it would have cost if I ordered a call drink!

;^o


[...]

[David Brown]

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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 :-)

[Richard Harnden]

Post by Kaz Kylheku

Post by Janis Papanagnou

Post by Kaz Kylheku
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)

[Lawrence D'Oliveiro]

Post by David Brown
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.

[Janis Papanagnou]

Post by Lawrence D'Oliveiro

Post by David Brown
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

Post by Lawrence D'Oliveiro
They can’t all be right on the first point, they can all be right on the
second.

[Scott Lurndal]

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Chris M. Thomasson
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.)

Post by Kaz Kylheku
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.

[Tim Rentsch]

Post by Ben Bacarisse

Post by Anton Shepelev

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
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
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;
}
}
}
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 );
}

[Phil Carmody]

Post by Ben Bacarisse

Post by Anton Shepelev

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
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
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;
}

Post by Ben Bacarisse
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.

Post by Ben Bacarisse
}
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

[Ben Bacarisse]

Post by Phil Carmody

Post by Ben Bacarisse

Post by Anton Shepelev

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
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
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
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.

Post by Phil Carmody

Post by Ben Bacarisse
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.

Post by Phil Carmody
Again, early
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...

Post by Phil Carmody
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.

Post by Phil Carmody
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.

Post by Phil Carmody

Post by Ben Bacarisse
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.

Post by Phil Carmody
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);

!

Post by Phil Carmody
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.

[Lawrence D'Oliveiro]

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.

[Phil Carmody]

Post by Lawrence D'Oliveiro

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

[Kaz Kylheku]

Post by Ben Bacarisse

Post by Thiago Adams

Post by Thiago Adams
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...]

Post by Thiago Adams
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.

[Anton Shepelev]

Post by Thiago Adams
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.

[Kaz Kylheku]

Post by Anton Shepelev

Post by Thiago Adams
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.

[Kaz Kylheku]

Post by Kaz Kylheku

Post by Anton Shepelev

Post by Thiago Adams
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.

[Anton Shepelev]

Post by Kaz Kylheku
You generally want parallel elements at the same level of
indentation.

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

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

Yes.

Post by Kaz Kylheku
This is just an example; I realize we are not handling the
case of the key being found.
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);

Post by Kaz Kylheku
The structure being recommended by Anton is most at home
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.

[Janis Papanagnou]

Post by Kaz Kylheku

Post by Anton Shepelev

Post by Thiago Adams
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.

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

Post by Kaz Kylheku
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.

[Kaz Kylheku]

Post by Janis Papanagnou

Post by Kaz Kylheku

Post by Anton Shepelev

Post by Thiago Adams
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.

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

Post by Kaz Kylheku
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.

[Thiago Adams]

Link for the sample using GCC and CLANG (no warnings in both)

GCC https://godbolt.org/z/rGre3hbE1

CLANG https://godbolt.org/z/ez114sEMW

For this other sample, the compiler needs to track the state of p inside
conditional expressions. This check is wrong (p || p->text) cake and GCC
shows that.

#pragma safety enable

#include <stdlib.h>
#include <string.h>

struct X {
char * _Owner _Opt text;
};

int main() {
struct X * _Owner _Opt p = calloc(1, sizeof * p);
if (p || p->text){
p->text = strdup("a");
}
free(p->text);
free(p);
}

cake

http://thradams.com/cake/playground.html?code=CiNwcmFnbWEgc2FmZXR5IGVuYWJsZSAKCiNpbmNsdWRlIDxzdGRsaWIuaD4KI2luY2x1ZGUgPHN0cmluZy5oPgoKc3RydWN0IFggewogIGNoYXIgKiBfT3duZXIgX09wdCB0ZXh0Owp9OwoKaW50IG1haW4oKSB7ICAgCiAgIHN0cnVjdCBYICogX093bmVyIF9PcHQgcCA9IGNhbGxvYygxLCBzaXplb2YgKiBwKTsKICAgaWYgKHAgfHwgcC0%2BdGV4dCl7ICAgCiAgICAgcC0%2BdGV4dCA9IHN0cmR1cCgiYSIpOyAgICAgCiAgIH0KICAgZnJlZShwLT50ZXh0KTsKICAgZnJlZShwKTsgIAp9CgoKCg%3D%3D&to=-1&options=

GCC
https://godbolt.org/z/4jdc7r9r3

GCC
does NOT detects this one

https://godbolt.org/z/T6GhfzrKT

and cake does

http://thradams.com/cake/playground.html?code=CiNwcmFnbWEgc2FmZXR5IGVuYWJsZSAKCiNpbmNsdWRlIDxzdGRsaWIuaD4KI2luY2x1ZGUgPHN0cmluZy5oPgoKc3RydWN0IFggewogIGNoYXIgKiBfT3duZXIgX09wdCB0ZXh0Owp9OwoKaW50IG1haW4oKSB7ICAgCiAgIHN0cnVjdCBYICogX093bmVyIF9PcHQgcCA9IGNhbGxvYygxLCBzaXplb2YgKiBwKTsKICAgaWYgKHAgKXsgICAKICAgICBwLT50ZXh0ID0gc3RyZHVwKCJhIik7ICAgICAKICAgfQogICAvL2ZyZWUocC0%2BdGV4dCk7CiAgIGZyZWUocCk7ICAKfQoKCgo%3D&to=-1&options=

[Lawrence D'Oliveiro]

Post by Thiago Adams
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*/

[Lawrence D'Oliveiro]

Post by Lawrence D'Oliveiro
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*/