On Mon, Apr 28, 2014 at 11:44 PM, Andreas Färber <afaer...@suse.de> wrote: > 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. >
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 >