Hi Jan,
> On 16 Feb 2022, at 14:43, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 16.02.2022 15:35, Bertrand Marquis wrote:
>> Hi Jan,
>>
>>> On 16 Feb 2022, at 14:03, Jan Beulich <jbeul...@suse.com> wrote:
>>>
>>> On 16.02.2022 14:57, Bertrand Marquis wrote:
>>>>> On 16 Feb 2022, at 12:23, George Dunlap <george.dun...@citrix.com> wrote:
>>>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>>>> On 16.02.2022 12:34, George Dunlap wrote:
>>>>>>> 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.
>>>>>
>>>>> I see what you’re saying with AND/OR; I personally still prefer OR but
>>>>> wouldn’t argue to hard against AND if others preferred it.
>>>>>
>>>>> As far as I’m concerned, the fact that we’re reducing lines of code isn’t
>>>>> a reason to use this at all. As our CODING_STYLE says, ASSERT() is just
>>>>> a louder printk. We would never consider writing PRINTK_AND_RETURN(),
>>>>> and we would never consider writing a macro like CONDRET(condition,
>>>>> retval) to replace
>>>>>
>>>>> if (condition)
>>>>> return retval;
>>>>>
>>>>> The only justification for this kind of macro, in my opinion, is to avoid
>>>>> duplication errors; i.e. replacing your code segment with the following:
>>>>>
>>>>> if (condition) {
>>>>> ASSERT(!condition);
>>>>> return foo;
>>>>> }
>>>>>
>>>>> is undesirable because there’s too much risk that the conditions will
>>>>> drift or be inverted incorrectly. But having control statements like
>>>>> ‘return’ and ‘continue’ in a macro is also undesirable in my opinion; I’m
>>>>> personally not sure which I find most undesirable.
>>>>>
>>>>> I guess one advantage of something like ASSERT_OR(condition, return foo);
>>>>> or ASSERT_OR(condition, continue); is that searching for “return” or
>>>>> “continue” will come up even if you’re doing a case-sensitive search.
>>>>> But I’m still wary of unintended side effects.
>>>>>
>>>>> Bertrand / Julien, any more thoughts?
>>>>
>>>> I think that having macros which are magic like that one which includes a
>>>> possible return and the fact that the macro is taking code as argument is
>>>> making the code really hard to read and understand for someone not knowing
>>>> this.
>>>> Even the code is longer right now, it is more readable and easy to
>>>> understand which means less chance for errors so I do not think the macro
>>>> will avoid errors but might in fact introduce some in the future.
>>>>
>>>> So I am voting to keep the current macro as it is.
>>>
>>> But you recall that there were two aspects to me wanting the switch?
>>> (Source) code size was only one. The other was that ASSERT_UNREACHABLE()
>>> doesn't show the original expression which has triggered the failure,
>>> unlike ASSERT() does.
>>
>> Sorry I focused on the macro part after Julien asked me to comment from the
>> Fusa point of view.
>>
>> The usual expectation is that an ASSERT should never occur and is an help
>> for the programmer to detect programming errors. Usually an assert is
>> crashing the application with an information of where an assert was
>> triggered.
>> In the current case, the assert is used as debug print and in production
>> mode an error is returned if this is happening without any print. Isn’t this
>> a debug print case instead of an assert ?
>
> Depends on the terminology you want to use: Our ASSERT() is in no way
> different in this regard from the C standard's assert(). The message
> logged is of course to aid the developers. But personally I'd call it
> more than just a "debug print".
But it will be if we change it. But I agree with you it is more than a debug
print.
Bertrand
>
> Jan
