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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

Jan