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

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

Lawrence D'Oliveiro:
>

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

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

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

I think you forget to set
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;
}

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

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

--
Ben.