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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

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

Bertrand

> 
> -George