Message ID | 1489638621-31978-1-git-send-email-rimjhim0107@gmail.com |
---|---|
State | New |
Headers | show |
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH] puv3: always compile-check debug printf Message-id: 1489638621-31978-1-git-send-email-rimjhim0107@gmail.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 016f432 puv3: always compile-check debug printf === OUTPUT BEGIN === Checking PATCH 1/1: puv3: always compile-check debug printf... ERROR: else should follow close brace '}' #32: FILE: include/hw/unicore32/puv3.h:50: + } + else { total: 1 errors, 0 warnings, 19 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
Anishka0107 <rimjhim0107@gmail.com> writes: > To prevent bitrot of the format string of the debug statement, files with > conditional debug statements should ensure that printf is compiled always, > and enclosed within if(0) statements and not in #ifdef. > > Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com> > --- > include/hw/unicore32/puv3.h | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h > index 5a4839f..e268484 100644 > --- a/include/hw/unicore32/puv3.h > +++ b/include/hw/unicore32/puv3.h > @@ -41,10 +41,14 @@ > #define PUV3_IRQS_OST0 (26) > > /* All puv3_*.c use DPRINTF for debug. */ > -#ifdef DEBUG_PUV3 > -#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__) > -#else > -#define DPRINTF(fmt, ...) do {} while (0) > -#endif > +#define DEBUG_PUV3 0 > + > +#define DPRINTF(fmt, ...) > + if (DEBUG_PUV3) { > + fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__) > + } > + else { > + do {} while (0) > + } This is incorrect. It's fine not to have an else leg as the compiler will dead-code away the if (0) part. However you still need to wrap in a do {} while to avoid problems with trailing ;'s. Also you need line continuations for defining macros. Did you actually compile test this patch? I suspect it wouldn't build due to the missing ;s for your fprintf and do while. See hw/misc/imx6_src.c for a debug printf I recently converted. > > #endif /* QEMU_HW_PUV3_H */ -- Alex Bennée
On 03/16/2017 07:04 AM, Alex Bennée wrote: > > Anishka0107 <rimjhim0107@gmail.com> writes: > >> To prevent bitrot of the format string of the debug statement, files with >> conditional debug statements should ensure that printf is compiled always, >> and enclosed within if(0) statements and not in #ifdef. >> >> Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com> >> --- >> include/hw/unicore32/puv3.h | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h >> index 5a4839f..e268484 100644 >> --- a/include/hw/unicore32/puv3.h >> +++ b/include/hw/unicore32/puv3.h >> @@ -41,10 +41,14 @@ >> #define PUV3_IRQS_OST0 (26) >> >> /* All puv3_*.c use DPRINTF for debug. */ >> -#ifdef DEBUG_PUV3 >> -#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__) >> -#else >> -#define DPRINTF(fmt, ...) do {} while (0) >> -#endif >> +#define DEBUG_PUV3 0 >> + >> +#define DPRINTF(fmt, ...) >> + if (DEBUG_PUV3) { >> + fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__) >> + } >> + else { >> + do {} while (0) >> + } > > This is incorrect. It's fine not to have an else leg as the compiler > will dead-code away the if (0) part. However you still need to wrap in a > do {} while to avoid problems with trailing ;'s. Also you need line > continuations for defining macros. > > Did you actually compile test this patch? I suspect it wouldn't build > due to the missing ;s for your fprintf and do while. On top of what Alex pointed out, I think "PATCH v2" you posted is a fix to this one. You should always post a complete patch for review. If you compile QEMU with DEBUG_PUV3 enabled, you will notice compilation errors in hw/dma/puv3_dma.c. Maybe you can fix them altogether. qemu-upstream.git/include/hw/unicore32/puv3.h:46:34: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr {aka long unsigned int}’ [-Werror=format=] #define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__) ^ hw/dma/puv3_dma.c:47:5: note: in expansion of macro ‘DPRINTF’ DPRINTF("offset 0x%x, value 0x%x\n", offset, ret); ^~~~~~~ > > See hw/misc/imx6_src.c for a debug printf I recently converted. > >> >> #endif /* QEMU_HW_PUV3_H */ > > > -- > Alex Bennée >
diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h index 5a4839f..e268484 100644 --- a/include/hw/unicore32/puv3.h +++ b/include/hw/unicore32/puv3.h @@ -41,10 +41,14 @@ #define PUV3_IRQS_OST0 (26) /* All puv3_*.c use DPRINTF for debug. */ -#ifdef DEBUG_PUV3 -#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__) -#else -#define DPRINTF(fmt, ...) do {} while (0) -#endif +#define DEBUG_PUV3 0 + +#define DPRINTF(fmt, ...) + if (DEBUG_PUV3) { + fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__) + } + else { + do {} while (0) + } #endif /* QEMU_HW_PUV3_H */
To prevent bitrot of the format string of the debug statement, files with conditional debug statements should ensure that printf is compiled always, and enclosed within if(0) statements and not in #ifdef. Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com> --- include/hw/unicore32/puv3.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)