Path: ...!feeds.phibee-telecom.net!2.eu.feeder.erje.net!feeder.erje.net!eternal-september.org!feeder3.eternal-september.org!news.eternal-september.org!.POSTED!not-for-mail
From: David Brown <david.brown@hesbynett.no>
Newsgroups: comp.lang.c
Subject: Re: Fixing a sample from K&R book using cake static analyser
Date: Mon, 24 Jun 2024 11:39:49 +0200
Organization: A noiseless patient Spider
Lines: 67
Message-ID: <v5bet5$s3j6$1@dont-email.me>
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> <v5aeue$j1nj$4@dont-email.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Injection-Date: Mon, 24 Jun 2024 11:39:49 +0200 (CEST)
Injection-Info: dont-email.me; posting-host="995058d498c537c0d70be20402ad0eb4";
	logging-data="921190"; mail-complaints-to="abuse@eternal-september.org";	posting-account="U2FsdGVkX19vJjbk71I74f5YCxMHNEJXPz691nqBepM="
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Cancel-Lock: sha1:yAJkkN2sOJ28NrNLgE3BO2krid0=
In-Reply-To: <v5aeue$j1nj$4@dont-email.me>
Content-Language: en-GB
Bytes: 3924

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.