Re: [Qemu-devel] [RFC PATCH] osdep.h: Prohibit disabling assert() in supported builds

[Thomas Huth]

On 05.09.2017 21:50, Eric Blake wrote:
> On 08/24/2017 02:51 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> On 08/22/2017 06:19 AM, Halil Pasic wrote:
[...]
>>> I'd prefer that if we are going to introduce our own construct that
>>> always evaluates side effects, and only has a compile-time switch on
>>> whether to abort() or (foolishly) plow on, that we name it something
>>> without 'assert' in the name, so that reviewers don't have to be
>>> confused about remembering which variant evaluates side effects.  Maybe:
>>>
>>> q_verify(cond)
>>
>> I vote for frying bigger fish.
>>
>> I also vote for using standard C when standard C is servicable.
> 
> So if it were up to me alone, the answer is:
> 
> I'm NOT going to add any new construct (whether spelled q_verify() or
> otherwise), and will merely document in the commit message that we
> discussed this as an alternative (so someone who wants to disable #error
> can get a git history of what went into the decision).
> 
> Also, it sounds like we want to keep it #error, not #warn.
> 
> But if anyone else has strong opinions before we promote this from RFC
> to actual patch, I'm still interested in your arguments.

You asked for opinions, so here's mine: I agree with you, please do
*not* add a new QEMU-specific construct here. assert() should be a
well-known C construct that every programmer should have understood. You
also need it for other projects. If you haven't understood that it's a
macro and has side-effects, you should learn it (e.g. during patch
review), not avoid it, otherwise you'll run into problems in another
project that is using it again.

But IMHO we should still try to get rid of wrong usage of assert() in
the QEMU sources. So maybe we could allow building with NDEBUG one day
for the brave people who need the extra percent of additional speed. But
as long as we're not there, I think this patch is a good thing to avoid
wrong expectations.

 Thomas

signature.asc
Description: OpenPGP digital signature