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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan