Re: [Qemu-devel] [PATCH 01/14] dma: Convert conditional compilation of debug printfs to regular ifs

[Peter Crosthwaite]

On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber <address@hidden> wrote:
> Am 28.04.2014 14:41, schrieb Peter Crosthwaite:
>> On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <address@hidden> wrote:
>>> Hi Marc,
>>>
>>> Am 28.04.2014 10:26, schrieb Marc Marí:
>>>> From: Marc Marí <address@hidden>
>>>>
>>>> Modify debug macros as explained in 
>>>> https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html
>>>>
>>>> Signed-off-by: Marc Marí <address@hidden>
>>>> ---
>>>>  hw/dma/i82374.c |   17 ++++++++++-------
>>>>  hw/dma/i8257.c  |   24 +++++++++++++++++-------
>>>>  hw/dma/rc4030.c |   13 +++++++++----
>>>>  3 files changed, 36 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
>>>> index dc7a767..fff4e6f 100644
>>>> --- a/hw/dma/i82374.c
>>>> +++ b/hw/dma/i82374.c
>>>> @@ -24,15 +24,18 @@
>>>>
>>>>  #include "hw/isa/isa.h"
>>>>
>>>> -//#define DEBUG_I82374
>>>> +//#define DEBUG_I82374 1
>>>>
>>>> -#ifdef DEBUG_I82374
>>>> -#define DPRINTF(fmt, ...) \
>>>> -do { fprintf(stderr, "i82374: " fmt , ## __VA_ARGS__); } while (0)
>>>> -#else
>>>> -#define DPRINTF(fmt, ...) \
>>>> -do {} while (0)
>>>> +#ifndef DEBUG_I82374
>>>> +#define DEBUG_I82374 0
>>>>  #endif
>>>
>>> This is exactly how I told you not to do it in response to Peter C.'s
>>> proposal. I had done so in my v1 [1] and it was rejected.
>>>
>>> Instead it was concluded that we need:
>>>
>>> //#define DEBUG_FOO
>>>
>>> #ifdef DEBUG_FOO
>>> #define DEBUG_FOO_ENABLED 1
>>> #else
>>> #define DEBUG_FOO_ENABLED 2
>
> Oops, obviously this what meant to read 0, not 2, i.e. translating
> absence of a constant to another constant that is assured to always
> carry a value.
>
> true/false may be an alternative for boolean logic.
>
>>> #endif
>>>
>>
>> if you are going to go this way you probably want:
>>
>> #ifdef DEBUG_FOO
>> #define DEBUG_FOO_ENABLED DEBUG_FOO
>> #else
>> #define DEBUG_FOO_ENABLED 0
>> #endif
>
> Is it guaranteed that #define DEBUG_FOO => DEBUG_FOO == 1? I assume so.

well -DDEBUG_FOO as a CFLAG comes with this == 1 guarantee which is
the prescribed usage. If you want to hack source with #define then
it's local to the code and your compile failure will quickly
straighten you out.

> Otherwise this may be just applicable to code where you do this kind of
> level-based distinction rather than presence/absence.
>

I prefer always level-capable - It's nice to be able to quickly add a
higher level of debug without applying a framework change.

> The real question to ask is, does the code have any #ifdef DEBUG_FOO, or
> does the respective maintainer intend to use it that way? If not, then
> your if (DEBUG_FOO) {...} is perfectly valid and makes more sense than
> having ..._ENABLED be anything but a boolean. It's just totally unsafe
> to assume this to work everywhere in one huge cross-maintainer
> refactoring series, as my earlier series showed (which did locate and
> update such #ifdef DEBUG_FOO code). The series becomes non-trivial then.
>

Your change is a good idea perhaps even universally. Just I think
levels are a valuable feature.

Something else to consider as well will be converting these #define
DEBUG_FOOs "further down the file" to regular if() too where possible
for the same reasons as this series.

Regards,
Peter

> Cheers,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>