Lluís Vilanova <vilan...@ac.upc.edu> writes:
> Markus Armbruster writes:
>
>> Lluís Vilanova <vilan...@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>>
>>>> Lluís Vilanova <vilan...@ac.upc.edu> 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 <vilan...@ac.upc.edu>
>>>>>
>>>>> 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.
[...]
