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: Code guidelines
Date: Tue, 3 Sep 2024 16:49:48 +0200
Organization: A noiseless patient Spider
Lines: 147
Message-ID: <vb77mc$3bu07$2@dont-email.me>
References: <vb6v1t$3b5mb$1@dont-email.me> <vb726n$3b4rq$1@dont-email.me>
 <vb736j$3b5mb$2@dont-email.me> <vb75c5$3bu07$1@dont-email.me>
 <vb76us$3bntp$2@dont-email.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Injection-Date: Tue, 03 Sep 2024 16:49:49 +0200 (CEST)
Injection-Info: dont-email.me; posting-host="93d7d6b8e3b40fe75b60c4526d162769";
	logging-data="3536903"; mail-complaints-to="abuse@eternal-september.org";	posting-account="U2FsdGVkX1/UdOnCKr5bpjHaTo90M158owMul+kjUs4="
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Cancel-Lock: sha1:4HwXP6u8Bml/6DMWVMPFLl14Q1M=
Content-Language: en-GB
In-Reply-To: <vb76us$3bntp$2@dont-email.me>
Bytes: 6797

On 03/09/2024 16:37, Thiago Adams wrote:
> On 03/09/2024 11:10, David Brown wrote:
>> On 03/09/2024 15:33, Thiago Adams wrote:
>>> On 03/09/2024 10:16, David Brown wrote:
>>>> On 03/09/2024 14:22, Thiago Adams wrote:
>>
>>>> I am of the opinion that if something is clearly specified, you make 
>>>> sure it is true when you are responsible for it - and then you can 
>>>> assume it is true when you use the results.  It makes no sense to me 
>>>> to do something, and then immediately check that it is done.
>>>>
>>>> Whenever possible, these specifications should be in code, not 
>>>> comments - using whatever tools and extra features you have 
>>>> available for use. Macros and conditional compilation help make code 
>>>> more portable, and can also let you turn compile-time assumptions 
>>>> into run-time checks during debugging.
>>>>
>>>
>>> Yes, this contract "function don't returns null" is very 
>>> straightforward for the caller and for the implementer.
>>>
>>> The contract implementation can be checked inside function 
>>> implementation. (One place to check the contract implementation)
>>> The contract usage, is checked in each caller, but very straightforward.
>>>
>>> On the other hand, struct members have much more places where the 
>>> contract can be broken. Each function that have a non const access to 
>>> user->name could break the contract implementation.
>>> The risk is much bigger compared with the return case.
>>
>> You are asking about "guidelines".  Yes, it is possible for code to 
>> break specifications.  The guideline is "don't do that".
>>
> 
> The guidelines are more about the usage of runtime checks, are they 
> required? optional? etc.. when to use etc...
> 

The guideline is "follow the specifications".  Then it is obvious that 
run-time checks are not required unless you are dealing with code that 
does not follow the specifications.

Now, as you note below, code under development might not follow the 
specifications - that is why it is useful to do this sort of thing with 
macros that support adding run-time checks during development or 
debugging.  Standard C "assert" is a poor man's version of this.

> 
>> If you are mixing untrusted code with trusted code, then you have to 
>> check the incoming data just like you do with data from external 
>> sources.  But most functions are not at such boundaries.
> 
> yes ..agree.
> 
>>>
>>> So I think it may make sense to have redundant runtime checks, but it 
>>> must be clear the the "compile time contract" is not that one.
>>>
>>>
>>> For instance:
>>>
>>> The first sample my create confusion (is name optional?)
>>>
>>> void f(struct user* user)
>>> {
>>>       if (user->name && strcmp(user->name, "john") == 0)
>>>       {
>>>          //...
>>>       }
>>>   }
>>>
>>> But :
>>> void f(struct user* user)
>>> {
>>>       assert(user->name);
>>>       if (user->name && strcmp(user->name, "john") == 0)
>>>       {
>>>          //...
>>>       }
>>>   }
>>>
>>> would show redundancy but making clear the contract still "name 
>>> should not be null"
>>>
>>
>> No, redundant runtime checks are not helpful.  If they are redundant, 
>> they are by definition a waste of effort - either run-time efficiency, 
>> or developer efficiency, or both.  And they are a maintenance mess as 
>> changes have to be applied in multiple places.
>>
> 
> But we know , when the code is new (under development) the contract may 
> not be so clear or change very frequently.
> 

Then it is even worse to have redundant checks - when they are 
inconsistent, you have no idea what is going on or what /should/ be 
going on.


> 
>>      void f(struct user* user)
>>      {
>>          if (!user->name) __builtin_unreachable();
>>          if (strcmp(user->name, "john") == 0) {
>>              //...
>>          }
>>      }
>>
>> Now it is /really/ clear that user->name should not be null, and 
>> instead of two run-time checks doing the same thing, there are no 
>> run-time costs and the compiler may even be able to optimise more from 
>> the knowledge that the pointer is not null.
>>
>> In practice I use a macro called "Assume" here, which will give a hard 
>> compile-time error if the compiler can that the assumption is false.  
>> It also supports optional run-time checks depending on a #define'd 
>> flag (normally set as a -D compiler flag), as an aid to debugging.
>>
>> Write things clearly, once, in code.
>>
>> If the code is boundary code, then you need a check - but then a 
>> simple null pointer check is unlikely to be sufficient.  (What if the 
>> pointer is not null, but not a valid pointer?  Or it does not point to 
>> a string of a suitable length, with suitable character set, etc.?)
>>
> 
> The macro I need is something like
> 
> "I know this check is redundant, but I will add it anyway" "this check 
> is redundant but according with the contract"

If a check is redundant, and the compiler can see that, the extra check 
will be eliminated by optimisation.  So you are wasting developer time 
for nothing.

> 
> assert/assume is more
> 
> "trust me compiler, this is what happens" someone else is checking this 
> we don't need to check again.
> 

That is not what "assert" is, but it is roughly what my "Assume" is. 
And yes, that is what you should be aiming for in the code.