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

[Jan Beulich <jbeulich@suse.com>]

On 16.02.2022 12:34, George Dunlap wrote:
>> On Feb 16, 2022, at 9:31 AM, Jan Beulich <jbeulich@suse.com> wrote:
>> On 16.02.2022 10:25, Bertrand Marquis wrote:
>>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xen.org> wrote:
>>>> On 27/01/2022 14:34, Jan Beulich wrote:
>>>>> The increasing amount of constructs along the lines of
>>>>>    if ( !condition )
>>>>>    {
>>>>>        ASSERT_UNREACHABLE();
>>>>>        return;
>>>>>    }
>>>>> is not only longer than necessary, but also doesn't produce incident
>>>>> specific console output (except for file name and line number).
>>>>
>>>> So I agree that this construct will always result to a minimum 5 lines. Which is not nice. But the proposed change is...
>>>>
>>>>> Allow
>>>>> the intended effect to be achieved with ASSERT(), by giving it a second
>>>>> parameter allowing specification of the action to take in release builds
>>>>> in case an assertion would have triggered in a debug one. The example
>>>>> above then becomes
>>>>>    ASSERT(condition, return);
>>>>> Make sure the condition will continue to not get evaluated when just a
>>>>> single argument gets passed to ASSERT().
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> v2: Rename new macro parameter.
>>>>> ---
>>>>> RFC: The use of a control flow construct as a macro argument may be
>>>>>     controversial.
>>>>
>>>> indeed controversial. I find this quite confusing and not something I would request a user to switch to if they use the longer version.
>>>>
>>>> That said, this is mainly a matter of taste. So I am interested to hear others view.
>>>>
>>>> I have also CCed Bertrand to have an opinions from the Fusa Group (I suspect this will go backward for them).
>>>
>>> Thanks and here is my feedback in regards to Fusa here.
>>>
>>> Most certification standards are forbidding completely macros including
>>> conditions (and quite a number are forbidding static inline with conditions).
>>> The main reason for that is MCDC coverage (condition/decisions and not only
>>> code line) is not possible to do anymore down to the source code and has to be
>>> done down to the pre-processed code.
>>>
>>> Out of Fusa considerations, one thing I do not like in this solution is the fact that
>>> you put some code as parameter of the macro (the return).
>>>
>>> To make this a bit better you could put the return code as parameter
>>> instead of having “return CODE” as parameter.
>>
>> Except that it's not always "return" what you want, and hence it
>> can't be made implicit.
>>
>>> An other thing is that Xen ASSERT after this change will be quite different from
>>> any ASSERT found in other projects which could make it misleading for developers.
>>> Maybe we could introduce an ASSERT_RETURN macros instead of modifying the
>>> behaviour of the standard ASSERT ?
>>
>> Along the lines of the above, this would then mean a multitude of
>> new macros.
> 
> Out of curiosity, what kinds of other actions?

continue, break, assignments of e.g. error codes, just to name a
few immediately obvious ones.

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

Jan