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

[Andreas Färber]

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

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.

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