> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeul...@suse.com> 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
signature.asc
Description: Message signed with OpenPGP
