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

[Julien Grall <julien@xxxxxxx>]

Hi George,

Sorry for the late answer.

On 16/02/2022 12:23, George Dunlap 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?
The discussion reminds me WARN_ONCE() in Linux. The macro returns a 
value so it can be used like:
if (WARN_ONCE(...))
  return error;

How about introducing a new macro that would return whether the check 
passed and if the check failed crashed in debug build?
I am not suggesting to modify ASSERT() because the compiler may decide 
to not ellide check in production build. Also, the name feels a little 
bit odd.
Cheers,

--
Julien Grall