Path: ...!feed.opticnetworks.net!eternal-september.org!feeder3.eternal-september.org!news.eternal-september.org!.POSTED!not-for-mail
From: Phil Carmody <pc+usenet@asdf.org>
Newsgroups: comp.lang.c
Subject: Re: Fixing a sample from K&R book using cake static analyser
Date: Tue, 25 Jun 2024 11:47:50 +0300
Organization: A noiseless patient Spider
Lines: 134
Message-ID: <874j9h8l3d.fsf@fatphil.org>
References: <v53sl1$35qt7$1@dont-email.me> <v558hv$3dskb$1@dont-email.me>
	<20240623034624.135@kylheku.com> <87wmmfq4if.fsf@bsb.me.uk>
	<20240624012527.8bbe16b96f5bfca10feadb5c@gmail.moc>
	<87zfrbnsvv.fsf@bsb.me.uk>
MIME-Version: 1.0
Content-Type: text/plain
Injection-Date: Tue, 25 Jun 2024 11:03:42 +0200 (CEST)
Injection-Info: dont-email.me; posting-host="90f5951342381adbfc1f206a9c22a83e";
	logging-data="1573582"; mail-complaints-to="abuse@eternal-september.org";	posting-account="U2FsdGVkX1/vn50/g4jZUCfg8DBLXGDm"
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
Cancel-Lock: sha1:Dl0+ntQCJ8QPGEOHC8tpD46m8kw=
	sha1:NCWqHLx3ArlBgCQxtPwhHq9djFU=
Bytes: 5574

Ben Bacarisse <ben@bsb.me.uk> writes:
> Anton Shepelev <anton.txt@gmail.moc> writes:
>
>> Ben Bacarisse:
>>
>>> I don't get why the goto crowd want to complicate it so
>>> much.
>>
>> Will someone post a goto-less that fixes what I perceive as
>> bugs in the original:
>>
>>   a.  the failure to free() a newly allocated nlist in case
>>       of a later error,
>>
>>   b.  the failure to free() a newly allocated np->name in
>>       case of a later error,
>>
>>   c.  the failure to keep np->defn unchaged if the
>>       allocation of the new defn value failed.
>>
>> And my perception be wrong, let us first establish the
>> actual bug(s).
>
> With the usual trepidation that this is untested (though I did compile
> it), I'd write something like:
>
> struct nlist *install(const char *name, const char *defn)
> {
>      struct nlist *node = lookup(name);
>      if (node) {
>           char *new_defn = strdup(defn);
>           if (new_defn) {
>                free(node->defn);
>                node->defn = new_defn;
>                return node;
>           }
>           else return NULL;
>      }

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

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

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

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

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

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

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

> }
>
> We have four cases:
>
>   node with the name found
>      new definition allocated
>      new definition not allocated
>   node with the name not found
>      new node, name and definition all allocated
>      not all of new node, name and definition allocated.

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

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

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

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