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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan