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