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