[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] lib: extend ASSERT()
On 16.02.2022 15:35, Bertrand Marquis wrote: > Hi Jan, > >> On 16 Feb 2022, at 14:03, Jan Beulich <jbeulich@xxxxxxxx> wrote: >> >> On 16.02.2022 14:57, Bertrand Marquis wrote: >>>> On 16 Feb 2022, at 12:23, George Dunlap <George.Dunlap@xxxxxxxxxx> wrote: >>>>> On Feb 16, 2022, at 11:42 AM, Jan Beulich <jbeulich@xxxxxxxx> 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". Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |