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

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

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

struct nlist *install(const char *name, const char *defn)
{
      struct nlist *node = lookup(name);
      if (node) {
           char *new_defn = strdup(defn);
           if (new_defn) {
                free(node->defn);
                node->defn = new_defn;
                return node;
           }
           else return NULL;
      }
      else {
           struct nlist *new_node = malloc(sizeof *new_node);
           char *new_name = strdup(name), *new_defn = strdup(defn);
           if (new_node && new_name && new_defn) {
                unsigned hashval = hash(name);
                new_node->name = new_name;
                new_node->defn = new_defn;
                new_node->next = hashtab[hashval];
                return hashtab[hashval] = new_node;
           }
           else {
                free(new_defn);
                free(new_name);
                free(new_node);
                return NULL;
           }
      }
}
>
We have four cases:
>
   node with the name found
      new definition allocated new definition not allocated
   node with the name not found
      new node, name and definition all allocated not all of new node,
      name and definition allocated.
>
We can very simply reason about all of these situations.

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

I disagree.  It's much clearer (to me) to separate the cases and deal with them individually.
I think the only disadvantage of doing that as a strategy is that you can sometimes end up with duplicated code.  If the duplications are significant, refactor them out as separate static functions.
Ben's code here is the first version I have seen where I have not thought "That code is horrible.  I'd have to think about it to see what it does."
I have two small complaints. I think it is a bad idea to have more than one variable declaration and initialisation stuffed in the same line unless they are /very/ tightly coupled, and I certainly don't like it if pointers are involved.  And I don't like returning (or using) the value of an assignment.  Basically, I prefer one line of code to do one thing at a time.  But while /I/ would split up those lines, I don't find it hard to see what Ben is doing in the code.