qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates
Date: Mon, 08 Feb 2016 09:58:23 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Lluís Vilanova <address@hidden> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <address@hidden> writes:
>>> Markus Armbruster writes:
>>> 
>>>> Lluís Vilanova <address@hidden> writes:
>>>>> Markus Armbruster writes:
>>>>> 
>>>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>>>> my patches into a respin of your series.  You can also respin on top
>>>>>> of my series.
>>>>> 
>>>>> Reviewed-by: Lluís Vilanova <address@hidden>
>>>>> 
>>>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>>>> 
>>>>> * The error.h comments point to assert() instead of abort() as a 
>>>>> replacement for
>>>>> error_setg(&error_abort) (while your HACKING says otherwise).
>>> 
>>>> Where?
>>> 
>>> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).
>
>> Got it.  error_setg()'s comment:
>
>>     Likewise, don't error_setg(&error_abort, ...), use assert().
>
>> This is advice on what to do.
>
>> HACKING:
>
>>     Note that &error_fatal is just another way to exit(1), and
>>     &error_abort is just another way to abort().
>
>> This tries to extend the admonition on exit() and abort() to
>> &error_fatal and &error_abort.  I'm open for better ways to word it.
>> However, I feel this section of HACKING should not go into detail on
>> what to do, but leave that to error.h.
>
> Right. I just wanted to say the error.h comment should be:
>
>      Likewise, don't error_setg(&error_abort, ...), use abort().
>
> To be consistent with HACKING (which implies the equivalent for error_abort is
> abort()). But that's just me thinking that assert will be omitted when NDEBUG 
> is
> defined :)

In my opinion, the error.h comment should point to assert(), not
abort(), because &error_abort and assert() both print information on the
error location, while abort() doesn't.  I *want* people to use assert().

But I'd rather not explain all that in HACKING, to avoid getting bogged
down in detail there.  Feel free to suggest a wording that improves
consistency without getting too far into the weeds.

HACKING doesn't say &error_abort is *equivalent* to abort(), it says
"&error_abort is just another way to abort()".  That's true for
assert(), too.  Except assert() comes with a compile time switch we
don't use.

Heh, just found this gem in hw/virtio/virtio.c:

    #ifdef NDEBUG
    #error building with NDEBUG is not supported
    #endif

At least it got a suitable TODO comment.

> As a side note (just an observation), it seems that NDEBUG is only selectively
> (and manually) defined in some files like "tcg/tcg.c", so it's not 
> consistently
> affecting files (e.g., it's used as a shorter substitute of "#ifdef
> CONFIG_DEBUG_TCG", but it's not acting like a "real" NDEBUG; aka assert will
> still work *iff* it's included before defining NDEBUG, which is not clear at
> all).

I can see two instances (tci.c tcg/tcg.c), and both look like this:

    #if !defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
    # define NDEBUG
    #endif

They make switching off CONFIG_DEBUG_TCG switch off assertions as well.
Feels odd to me, but I'm not familiar with these two files.

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]