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. 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. Regards, Peter > 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 >