Am 28.04.2014 15:35, schrieb Peter Crosthwaite: > On Mon, Apr 28, 2014 at 11:16 PM, Andreas Färber <afaer...@suse.de> wrote: >> Am 28.04.2014 14:41, schrieb Peter Crosthwaite: >>> On Mon, Apr 28, 2014 at 10:25 PM, Andreas Färber <afaer...@suse.de> wrote: >>>> Hi Marc, >>>> >>>> Am 28.04.2014 10:26, schrieb Marc Marí: >>>>> From: Marc Marí <5.markm...@gmail.com> >>>>> >>>>> Modify debug macros as explained in >>>>> https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html >>>>> >>>>> Signed-off-by: Marc Marí <5.markm...@gmail.com> >>>>> --- >>>>> 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