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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2022 12:34, George Dunlap wrote:
>> On Feb 16, 2022, at 9:31 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 16.02.2022 10:25, Bertrand Marquis wrote:
>>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xxxxxxx> 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@xxxxxxxx>
>>>>> ---
>>>>> 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