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

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

So you can implement leveled debugging with just the one symbol. See
m25p80.c for an example of two level debugging using the if() system.

Regards,
Peter

> or something like that, so that #ifdef DEBUG_FOO still works as expected
> further down the file.
>
> Otherwise the patch looks good, except for the missing space after if.
>
> Please add a cover letter 00/14 next time, so that general comments can
> be placed there and 01/14 with its replies gets shown alongside 02/14.
>
> Regards,
> Andreas
>
> [1] https://github.com/afaerber/qemu-cpu/commits/dprintf.v1
>
>> +
>> +#define DPRINTF(fmt, ...) \
>> +    do { \
>> +        if(DEBUG_I82374) { \
>> +            fprintf(stderr, "I82374: " fmt, ## __VA_ARGS__); \
>> +        } \
>> +    } while (0)
>>  #define BADF(fmt, ...) \
>>  do { fprintf(stderr, "i82374 ERROR: " fmt , ## __VA_ARGS__); } while (0)
>>
> [snip]
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>