[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] lib: extend ASSERT()
Hi Jan, > On 16 Feb 2022, at 14:43, Jan Beulich <jbeulich@xxxxxxxx> wrote: > > 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". But it will be if we change it. But I agree with you it is more than a debug print. Bertrand > > Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |