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:44 PM, Andreas Färber <address@hidden> wrote:
> 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.
>

Sounds good. I see your point. _LEVEL universally is my recommendation.

Regards,
Peter

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