Re: [PATCH v2] lib: extend ASSERT()

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 16 Feb 2022, at 14:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.02.2022 15:35, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 16 Feb 2022, at 14:03, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 16.02.2022 14:57, Bertrand Marquis wrote:
>>>>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@xxxxxxxxxx> wrote:
>>>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>>>>> I am opposed to overloading “ASSERT” for this new kind of macro; I 
>>>>>>> think it would not only be unnecessarily confusing to people not 
>>>>>>> familiar with our codebase, but it would be too easy for people to fail 
>>>>>>> to notice which macro was being used.
>>>>>>> 
>>>>>>> ASSERT_ACTION(condition, code) (or even ASSERT_OR_ACTION()) would be a 
>>>>>>> bare minimum for me.
>>>>>>> 
>>>>>>> But I can’t imagine that there are more than a handful of actions we 
>>>>>>> might want to take, so defining a macro for each one shouldn’t be too 
>>>>>>> burdensome.
>>>>>>> 
>>>>>>> Furthermore, the very flexibility seems dangerous; you’re not seeing 
>>>>>>> what actual code is generated, so it’s to easy to be “clever”, and/or 
>>>>>>> write code that ends up doing something different than you expect.
>>>>>>> 
>>>>>>> At the moment I think ASSERT_OR_RETURN(condition, code), plus other new 
>>>>>>> macros for the other behavior is needed, would be better.
>>>>>> 
>>>>>> Hmm, while I see your point of things possibly looking confusing or
>>>>>> unexpected, something like ASSERT_OR_RETURN() (shouldn't it be
>>>>>> ASSERT_AND_RETURN()?) is imo less readable. In particular I dislike
>>>>>> the larger amount of uppercase text. But yes, I could accept this
>>>>>> as a compromise as it still seems better to me than the multi-line
>>>>>> constructs we currently use.
>>>>> 
>>>>> I see what you’re saying with AND/OR; I personally still prefer OR but 
>>>>> wouldn’t argue to hard against AND if others preferred it.
>>>>> 
>>>>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t 
>>>>> a reason to use this at all.  As our CODING_STYLE says, ASSERT() is just 
>>>>> a louder printk.  We would never consider writing PRINTK_AND_RETURN(), 
>>>>> and we would never consider writing a macro like CONDRET(condition, 
>>>>> retval) to replace
>>>>> 
>>>>> if (condition)
>>>>>  return retval;
>>>>> 
>>>>> The only justification for this kind of macro, in my opinion, is to avoid 
>>>>> duplication errors; i.e. replacing your code segment with the following:
>>>>> 
>>>>> if (condition) {
>>>>>  ASSERT(!condition);
>>>>>  return foo;
>>>>> }
>>>>> 
>>>>> is undesirable because there’s too much risk that the conditions will 
>>>>> drift or be inverted incorrectly. But having control statements like 
>>>>> ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m 
>>>>> personally not sure which I find most undesirable.
>>>>> 
>>>>> I guess one advantage of something like ASSERT_OR(condition, return foo); 
>>>>> or ASSERT_OR(condition, continue); is that searching for “return” or 
>>>>> “continue” will come up even if you’re doing a case-sensitive search.  
>>>>> But I’m still wary of unintended side effects.
>>>>> 
>>>>> Bertrand / Julien, any more thoughts?
>>>> 
>>>> I think that having macros which are magic like that one which includes a 
>>>> possible return and the fact that the macro is taking code as argument is 
>>>> making the code really hard to read and understand for someone not knowing 
>>>> this.
>>>> Even the code is longer right now, it is more readable and easy to 
>>>> understand which means less chance for errors so I do not think the macro 
>>>> will avoid errors but might in fact introduce some in the future.
>>>> 
>>>> So I am voting to keep the current macro as it is.
>>> 
>>> But you recall that there were two aspects to me wanting the switch?
>>> (Source) code size was only one. The other was that ASSERT_UNREACHABLE()
>>> doesn't show the original expression which has triggered the failure,
>>> unlike ASSERT() does.
>> 
>> Sorry I focused on the macro part after Julien asked me to comment from the 
>> Fusa point of view.
>> 
>> The usual expectation is that an ASSERT should never occur and is an help 
>> for the programmer to detect programming errors. Usually an assert is 
>> crashing the application with an information of where an assert was 
>> triggered.
>> In the current case, the assert is used as debug print and in production 
>> mode an error is returned if this is happening without any print. Isn’t this 
>> a debug print case instead of an assert ?
> 
> Depends on the terminology you want to use: Our ASSERT() is in no way
> different in this regard from the C standard's assert(). The message
> logged is of course to aid the developers. But personally I'd call it
> more than just a "debug print".

But it will be if we change it. But I agree with you it is more than a debug 
print.

Bertrand

> 
> Jan