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

[Andreas Färber]

Am 28.04.2014 15:35, schrieb 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.

I wouldn't mind, but I suggest DEBUG_FOO or DEBUG_FOO_LEVEL naming then,
not DEBUG_FOO_ENABLED with numeric values please.

> 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.

Agreed, but either as follow-up patch/series for better review, or by
re-dividing this series along maintenance lines.

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