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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 16 Feb 2022, at 09:31, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.02.2022 10:25, Bertrand Marquis wrote:
>> Hi Jan, Julien,
>> 
>>> On 15 Feb 2022, at 21:00, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> (+ Bertrand)
>>> 
>>> Hi Jan,
>>> 
>>> 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.

Then having code as parameter for a macro is really not nice I think.

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

Understood then my statement of Xen having an ASSERT different from any other 
known
assert still stands, this is no something I would vote for.
If you want to keep the code then maybe using ASSERT_ACTION or something like 
that 
and keep ASSERT being a standard assert would be a good idea.

Regards
Bertrand

> 
> Jan