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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> 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?

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP