* [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup
@ 2017-11-30 13:41 Eric Blake
2017-11-30 13:41 ` [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0); Eric Blake
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Eric Blake @ 2017-11-30 13:41 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial
Noticed this by chance in the tests/ directory, so I broadened
it to a grep of the entire code base. I suspect many of the
bad macros were the victims of copy-and-paste from some other
bad location (particularly given how many bit-rotten debug
print macros were involved).
https://wiki.qemu.org/BiteSizedTasks#Bitrot_prevention is still
left for someone else, for another day.
Eric Blake (3):
net: Drop unusual use of do { } while (0);
mips: Tweak location of ';' in macros
maint: Fix macros with broken 'do/while(0);' usage
tests/acpi-utils.h | 8 ++++----
ui/sdl_zoom_template.h | 8 ++++----
audio/paaudio.c | 4 ++--
hw/adc/stm32f2xx_adc.c | 2 +-
hw/block/m25p80.c | 2 +-
hw/char/cadence_uart.c | 2 +-
hw/char/stm32f2xx_usart.c | 2 +-
hw/display/cg3.c | 2 +-
hw/display/dpcd.c | 2 +-
hw/display/xlnx_dp.c | 2 +-
hw/dma/pl330.c | 2 +-
hw/dma/xlnx-zynq-devcfg.c | 2 +-
hw/dma/xlnx_dpdma.c | 2 +-
hw/i2c/i2c-ddc.c | 2 +-
hw/misc/auxbus.c | 2 +-
hw/misc/macio/mac_dbdma.c | 4 ++--
hw/misc/mmio_interface.c | 2 +-
hw/misc/stm32f2xx_syscfg.c | 2 +-
hw/misc/zynq_slcr.c | 2 +-
hw/net/cadence_gem.c | 2 +-
hw/net/pcnet.c | 20 ++++++++++----------
hw/ssi/mss-spi.c | 2 +-
hw/ssi/stm32f2xx_spi.c | 2 +-
hw/ssi/xilinx_spi.c | 2 +-
hw/ssi/xilinx_spips.c | 2 +-
hw/timer/a9gtimer.c | 2 +-
hw/timer/cadence_ttc.c | 2 +-
hw/timer/mss-timer.c | 2 +-
hw/timer/stm32f2xx_timer.c | 2 +-
hw/tpm/tpm_passthrough.c | 2 +-
hw/tpm/tpm_tis.c | 2 +-
migration/rdma.c | 2 +-
target/arm/translate-a64.c | 2 +-
target/mips/msa_helper.c | 34 ++++++++++++++++++----------------
target/s390x/kvm.c | 2 +-
tests/tcg/test-mmap.c | 2 +-
36 files changed, 70 insertions(+), 68 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0);
2017-11-30 13:41 [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup Eric Blake
@ 2017-11-30 13:41 ` Eric Blake
2017-11-30 16:08 ` Thomas Huth
2017-11-30 13:41 ` [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros Eric Blake
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-11-30 13:41 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Jason Wang
For a couple of macros in pcnet.c, we have to provide a new scope
to avoid compiler warnings about declarations in the middle of a
switch statement that aren't in a sub-scope. But use of
'do { ... } while (0);' merely to provide that new scope is arcane
overkill, compared to just using '{ ... }'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
hw/net/pcnet.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 654455355f..641bf2fd88 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -455,32 +455,32 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
#define CHECK_RMD(ADDR,RES) do { \
switch (BCR_SWSTYLE(s)) { \
case 0x00: \
- do { \
+ { \
uint16_t rda[4]; \
s->phys_mem_read(s->dma_opaque, (ADDR), \
(void *)&rda[0], sizeof(rda), 0); \
(RES) |= (rda[2] & 0xf000)!=0xf000; \
(RES) |= (rda[3] & 0xf000)!=0x0000; \
- } while (0); \
+ } \
break; \
case 0x01: \
case 0x02: \
- do { \
+ { \
uint32_t rda[4]; \
s->phys_mem_read(s->dma_opaque, (ADDR), \
(void *)&rda[0], sizeof(rda), 0); \
(RES) |= (rda[1] & 0x0000f000L)!=0x0000f000L; \
(RES) |= (rda[2] & 0x0000f000L)!=0x00000000L; \
- } while (0); \
+ } \
break; \
case 0x03: \
- do { \
+ { \
uint32_t rda[4]; \
s->phys_mem_read(s->dma_opaque, (ADDR), \
(void *)&rda[0], sizeof(rda), 0); \
(RES) |= (rda[0] & 0x0000f000L)!=0x00000000L; \
(RES) |= (rda[1] & 0x0000f000L)!=0x0000f000L; \
- } while (0); \
+ } \
break; \
} \
} while (0)
@@ -488,22 +488,22 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
#define CHECK_TMD(ADDR,RES) do { \
switch (BCR_SWSTYLE(s)) { \
case 0x00: \
- do { \
+ { \
uint16_t xda[4]; \
s->phys_mem_read(s->dma_opaque, (ADDR), \
(void *)&xda[0], sizeof(xda), 0); \
(RES) |= (xda[2] & 0xf000)!=0xf000; \
- } while (0); \
+ } \
break; \
case 0x01: \
case 0x02: \
case 0x03: \
- do { \
+ { \
uint32_t xda[4]; \
s->phys_mem_read(s->dma_opaque, (ADDR), \
(void *)&xda[0], sizeof(xda), 0); \
(RES) |= (xda[1] & 0x0000f000L)!=0x0000f000L; \
- } while (0); \
+ } \
break; \
} \
} while (0)
--
2.14.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros
2017-11-30 13:41 [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup Eric Blake
2017-11-30 13:41 ` [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0); Eric Blake
@ 2017-11-30 13:41 ` Eric Blake
2017-11-30 15:33 ` Philippe Mathieu-Daudé
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
2017-11-30 14:01 ` [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style Eric Blake
3 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-11-30 13:41 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Aurelien Jarno, Yongbok Kim
It is more typical to provide the ';' by the caller of a macro
than to embed it in the macro itself; this is because syntax
highlight engines can get confused if a macro is called without
a semicolon before the closing '}'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
target/mips/msa_helper.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
index f167a42655..8fb7a369ca 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -682,13 +682,13 @@ static inline int64_t msa_mod_u_df(uint32_t df, int64_t arg1, int64_t arg2)
do { \
e = SIGNED_EVEN(a, df); \
o = SIGNED_ODD(a, df); \
- } while (0);
+ } while (0)
#define UNSIGNED_EXTRACT(e, o, a, df) \
do { \
e = UNSIGNED_EVEN(a, df); \
o = UNSIGNED_ODD(a, df); \
- } while (0);
+ } while (0)
static inline int64_t msa_dotp_s_df(uint32_t df, int64_t arg1, int64_t arg2)
{
@@ -1120,9 +1120,11 @@ void helper_msa_splat_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
#define MSA_LOOP_COND_D MSA_LOOP_COND(DF_DOUBLE)
#define MSA_LOOP(DF) \
+ do { \
for (i = 0; i < (MSA_LOOP_COND_ ## DF) ; i++) { \
- MSA_DO_ ## DF \
- }
+ MSA_DO_ ## DF; \
+ } \
+ } while (0)
#define MSA_FN_DF(FUNC) \
void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
@@ -1135,17 +1137,17 @@ void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
uint32_t i; \
switch (df) { \
case DF_BYTE: \
- MSA_LOOP_B \
+ MSA_LOOP_B; \
break; \
case DF_HALF: \
- MSA_LOOP_H \
+ MSA_LOOP_H; \
break; \
case DF_WORD: \
- MSA_LOOP_W \
+ MSA_LOOP_W; \
break; \
case DF_DOUBLE: \
- MSA_LOOP_D \
- break; \
+ MSA_LOOP_D; \
+ break; \
default: \
assert(0); \
} \
@@ -1168,7 +1170,7 @@ void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
do { \
R##DF(pwx, i) = pwt->DF[2*i]; \
L##DF(pwx, i) = pws->DF[2*i]; \
- } while (0);
+ } while (0)
MSA_FN_DF(pckev_df)
#undef MSA_DO
@@ -1176,7 +1178,7 @@ MSA_FN_DF(pckev_df)
do { \
R##DF(pwx, i) = pwt->DF[2*i+1]; \
L##DF(pwx, i) = pws->DF[2*i+1]; \
- } while (0);
+ } while (0)
MSA_FN_DF(pckod_df)
#undef MSA_DO
@@ -1184,7 +1186,7 @@ MSA_FN_DF(pckod_df)
do { \
pwx->DF[2*i] = L##DF(pwt, i); \
pwx->DF[2*i+1] = L##DF(pws, i); \
- } while (0);
+ } while (0)
MSA_FN_DF(ilvl_df)
#undef MSA_DO
@@ -1192,7 +1194,7 @@ MSA_FN_DF(ilvl_df)
do { \
pwx->DF[2*i] = R##DF(pwt, i); \
pwx->DF[2*i+1] = R##DF(pws, i); \
- } while (0);
+ } while (0)
MSA_FN_DF(ilvr_df)
#undef MSA_DO
@@ -1200,7 +1202,7 @@ MSA_FN_DF(ilvr_df)
do { \
pwx->DF[2*i] = pwt->DF[2*i]; \
pwx->DF[2*i+1] = pws->DF[2*i]; \
- } while (0);
+ } while (0)
MSA_FN_DF(ilvev_df)
#undef MSA_DO
@@ -1208,7 +1210,7 @@ MSA_FN_DF(ilvev_df)
do { \
pwx->DF[2*i] = pwt->DF[2*i+1]; \
pwx->DF[2*i+1] = pws->DF[2*i+1]; \
- } while (0);
+ } while (0)
MSA_FN_DF(ilvod_df)
#undef MSA_DO
#undef MSA_LOOP_COND
@@ -1222,7 +1224,7 @@ MSA_FN_DF(ilvod_df)
uint32_t k = (pwd->DF[i] & 0x3f) % (2 * n); \
pwx->DF[i] = \
(pwd->DF[i] & 0xc0) ? 0 : k < n ? pwt->DF[k] : pws->DF[k - n]; \
- } while (0);
+ } while (0)
MSA_FN_DF(vshf_df)
#undef MSA_DO
#undef MSA_LOOP_COND
--
2.14.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 13:41 [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup Eric Blake
2017-11-30 13:41 ` [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0); Eric Blake
2017-11-30 13:41 ` [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros Eric Blake
@ 2017-11-30 13:41 ` Eric Blake
2017-11-30 13:54 ` Cornelia Huck
` (3 more replies)
2017-11-30 14:01 ` [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style Eric Blake
3 siblings, 4 replies; 15+ messages in thread
From: Eric Blake @ 2017-11-30 13:41 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Gerd Hoffmann, Alistair Francis, Peter Crosthwaite,
Kevin Wolf, Max Reitz, Edgar E. Iglesias, Paolo Bonzini,
Peter Maydell, Alexander Graf, Jason Wang, Subbaraya Sundeep,
Stefan Berger, Juan Quintela, Dr. David Alan Gilbert,
Christian Borntraeger, Cornelia Huck, Richard Henderson,
Michael S. Tsirkin, Igor Mammedov, open list:Block layer core,
open list:Xilinx Zynq, open list:New World, open list:S390
The point of writing a macro embedded in a 'do { ... } while (0)'
loop is so that the macro can be used as a drop-in statement with
the caller supplying the trailing ';'. Although our coding style
frowns on brace-less 'if':
if (cond)
statement;
else
something else;
the use of do/while (0) in a macro is absolutely essential for the
purpose of avoiding a syntax error on the 'else' - but it only works
if there is no trailing ';' in the macro (as the ';' in the code
calling the macro would then be a second statement and cause the
'else' to not pair to the 'if').
Many of the places touched in this code are examples of the ugly
bit-rotting debug print statements; cleaning those up is left as
a bite-sized task for another day.
Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/acpi-utils.h | 8 ++++----
ui/sdl_zoom_template.h | 8 ++++----
audio/paaudio.c | 4 ++--
hw/adc/stm32f2xx_adc.c | 2 +-
hw/block/m25p80.c | 2 +-
hw/char/cadence_uart.c | 2 +-
hw/char/stm32f2xx_usart.c | 2 +-
hw/display/cg3.c | 2 +-
hw/display/dpcd.c | 2 +-
hw/display/xlnx_dp.c | 2 +-
hw/dma/pl330.c | 2 +-
hw/dma/xlnx-zynq-devcfg.c | 2 +-
hw/dma/xlnx_dpdma.c | 2 +-
hw/i2c/i2c-ddc.c | 2 +-
hw/misc/auxbus.c | 2 +-
hw/misc/macio/mac_dbdma.c | 4 ++--
hw/misc/mmio_interface.c | 2 +-
hw/misc/stm32f2xx_syscfg.c | 2 +-
hw/misc/zynq_slcr.c | 2 +-
hw/net/cadence_gem.c | 2 +-
hw/ssi/mss-spi.c | 2 +-
hw/ssi/stm32f2xx_spi.c | 2 +-
hw/ssi/xilinx_spi.c | 2 +-
hw/ssi/xilinx_spips.c | 2 +-
hw/timer/a9gtimer.c | 2 +-
hw/timer/cadence_ttc.c | 2 +-
hw/timer/mss-timer.c | 2 +-
hw/timer/stm32f2xx_timer.c | 2 +-
hw/tpm/tpm_passthrough.c | 2 +-
hw/tpm/tpm_tis.c | 2 +-
migration/rdma.c | 2 +-
target/arm/translate-a64.c | 2 +-
target/s390x/kvm.c | 2 +-
tests/tcg/test-mmap.c | 2 +-
34 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index d5ca5b6238..ac52abd0dd 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -32,7 +32,7 @@ typedef struct {
do { \
memread(addr, &field, sizeof(field)); \
addr += sizeof(field); \
- } while (0);
+ } while (0)
#define ACPI_READ_ARRAY_PTR(arr, length, addr) \
do { \
@@ -40,7 +40,7 @@ typedef struct {
for (idx = 0; idx < length; ++idx) { \
ACPI_READ_FIELD(arr[idx], addr); \
} \
- } while (0);
+ } while (0)
#define ACPI_READ_ARRAY(arr, addr) \
ACPI_READ_ARRAY_PTR(arr, sizeof(arr) / sizeof(arr[0]), addr)
@@ -56,7 +56,7 @@ typedef struct {
ACPI_READ_FIELD((table)->oem_revision, addr); \
ACPI_READ_ARRAY((table)->asl_compiler_id, addr); \
ACPI_READ_FIELD((table)->asl_compiler_revision, addr); \
- } while (0);
+ } while (0)
#define ACPI_ASSERT_CMP(actual, expected) do { \
char ACPI_ASSERT_CMP_str[5] = {}; \
@@ -77,7 +77,7 @@ typedef struct {
ACPI_READ_FIELD((field).bit_offset, addr); \
ACPI_READ_FIELD((field).access_width, addr); \
ACPI_READ_FIELD((field).address, addr); \
- } while (0);
+ } while (0)
uint8_t acpi_calc_checksum(const uint8_t *data, int len);
diff --git a/ui/sdl_zoom_template.h b/ui/sdl_zoom_template.h
index 3bb508b51e..6a424adfb4 100644
--- a/ui/sdl_zoom_template.h
+++ b/ui/sdl_zoom_template.h
@@ -34,22 +34,22 @@
#define setRed(r, pcolor) do { \
*pcolor = ((*pcolor) & (~(dpf->Rmask))) + \
(((r) & (dpf->Rmask >> dpf->Rshift)) << dpf->Rshift); \
-} while (0);
+} while (0)
#define setGreen(g, pcolor) do { \
*pcolor = ((*pcolor) & (~(dpf->Gmask))) + \
(((g) & (dpf->Gmask >> dpf->Gshift)) << dpf->Gshift); \
-} while (0);
+} while (0)
#define setBlue(b, pcolor) do { \
*pcolor = ((*pcolor) & (~(dpf->Bmask))) + \
(((b) & (dpf->Bmask >> dpf->Bshift)) << dpf->Bshift); \
-} while (0);
+} while (0)
#define setAlpha(a, pcolor) do { \
*pcolor = ((*pcolor) & (~(dpf->Amask))) + \
(((a) & (dpf->Amask >> dpf->Ashift)) << dpf->Ashift); \
-} while (0);
+} while (0)
static void glue(sdl_zoom_rgb, BPP)(SDL_Surface *src, SDL_Surface *dst, int smooth,
SDL_Rect *dst_rect)
diff --git a/audio/paaudio.c b/audio/paaudio.c
index 65beb6f010..2a35e6f82c 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -89,7 +89,7 @@ static inline int PA_STREAM_IS_GOOD(pa_stream_state_t x)
} \
goto label; \
} \
- } while (0);
+ } while (0)
#define CHECK_DEAD_GOTO(c, stream, rerror, label) \
do { \
@@ -107,7 +107,7 @@ static inline int PA_STREAM_IS_GOOD(pa_stream_state_t x)
} \
goto label; \
} \
- } while (0);
+ } while (0)
static int qpa_simple_read (PAVoiceIn *p, void *data, size_t length, int *rerror)
{
diff --git a/hw/adc/stm32f2xx_adc.c b/hw/adc/stm32f2xx_adc.c
index 90fe9de299..13f31ad2f7 100644
--- a/hw/adc/stm32f2xx_adc.c
+++ b/hw/adc/stm32f2xx_adc.c
@@ -37,7 +37,7 @@
if (STM_ADC_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index a2438b9ed2..62b5221bde 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -40,7 +40,7 @@
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/* Fields for FlashPartInfo->flags */
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 6143494060..fbdbd463bb 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -33,7 +33,7 @@
#define DB_PRINT(...) do { \
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
- } while (0);
+ } while (0)
#else
#define DB_PRINT(...)
#endif
diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 268e435338..07b462d4b6 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -34,7 +34,7 @@
if (STM_USART_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index e069c4484c..cafd9f47ef 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -63,7 +63,7 @@
if (DEBUG_CG3) { \
printf("CG3: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define TYPE_CG3 "cgthree"
#define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
index ce92ff6e2a..943002bee5 100644
--- a/hw/display/dpcd.c
+++ b/hw/display/dpcd.c
@@ -39,7 +39,7 @@
if (DEBUG_DPCD) { \
qemu_log("dpcd: " fmt, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define DPCD_READABLE_AREA 0x600
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 561f828e7a..ead4e1a0e4 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -34,7 +34,7 @@
if (DEBUG_DP) { \
qemu_log("xlnx_dp: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/*
* Register offset for DP.
diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 32cf8399b8..d071049233 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -29,7 +29,7 @@
if (PL330_ERR_DEBUG >= lvl) {\
fprintf(stderr, "PL330: %s:" fmt, __func__, ## args);\
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
index 3b10523430..12bb2e3716 100644
--- a/hw/dma/xlnx-zynq-devcfg.c
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -43,7 +43,7 @@
if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
REG32(CTRL, 0x00)
FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 8ceb21ddb3..077c7da9cc 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -34,7 +34,7 @@
if (DEBUG_DPDMA) { \
qemu_log("xlnx_dpdma: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/*
* Registers offset for DPDMA.
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index 6b92e95c73..199dac9e41 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -30,7 +30,7 @@
if (DEBUG_I2CDDC) { \
qemu_log("i2c-ddc: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/* Structure defining a monitor's characteristics in a
* readable format: this should be passed to build_edid_blob()
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 1182745044..b4cacd664b 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -40,7 +40,7 @@
if (DEBUG_AUX) { \
qemu_log("aux: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define TYPE_AUXTOI2C "aux-to-i2c-bridge"
#define AUXTOI2C(obj) OBJECT_CHECK(AUXTOI2CState, (obj), TYPE_AUXTOI2C)
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 0eddf2e700..1b2a69b3ef 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -52,7 +52,7 @@
if (DEBUG_DBDMA) { \
printf("DBDMA: " fmt , ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
if (DEBUG_DBDMA) { \
@@ -60,7 +60,7 @@
printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
} \
} \
-} while (0);
+} while (0)
/*
*/
diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
index 894e9801cb..3b0e2039a3 100644
--- a/hw/misc/mmio_interface.c
+++ b/hw/misc/mmio_interface.c
@@ -39,7 +39,7 @@ static uint64_t mmio_interface_counter;
if (DEBUG_MMIO_INTERFACE) { \
qemu_log("mmio_interface: 0x%" PRIX64 ": " fmt, s->id, ## __VA_ARGS__);\
} \
-} while (0);
+} while (0)
static void mmio_interface_init(Object *obj)
{
diff --git a/hw/misc/stm32f2xx_syscfg.c b/hw/misc/stm32f2xx_syscfg.c
index 7c45833d09..7f10195862 100644
--- a/hw/misc/stm32f2xx_syscfg.c
+++ b/hw/misc/stm32f2xx_syscfg.c
@@ -34,7 +34,7 @@
if (STM_SYSCFG_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index 44304d48be..d6bdd027ef 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -30,7 +30,7 @@
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
} \
- } while (0);
+ } while (0)
#define XILINX_LOCK_KEY 0x767b
#define XILINX_UNLOCK_KEY 0xdf0d
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3943187572..0fa4b0dc44 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -34,7 +34,7 @@
#define DB_PRINT(...) do { \
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
- } while (0);
+ } while (0)
#else
#define DB_PRINT(...)
#endif
diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
index d60daba882..185e1a3920 100644
--- a/hw/ssi/mss-spi.c
+++ b/hw/ssi/mss-spi.c
@@ -35,7 +35,7 @@
if (MSS_SPI_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt "\n", __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/ssi/stm32f2xx_spi.c b/hw/ssi/stm32f2xx_spi.c
index 26a1b4ddf5..69514da9fb 100644
--- a/hw/ssi/stm32f2xx_spi.c
+++ b/hw/ssi/stm32f2xx_spi.c
@@ -35,7 +35,7 @@
if (STM_SPI_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index 33482f04de..83585bc8b2 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -36,7 +36,7 @@
#define DB_PRINT(...) do { \
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
- } while (0);
+ } while (0)
#else
#define DB_PRINT(...)
#endif
diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index ef56d35f2c..c070393dc9 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -43,7 +43,7 @@
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/* config register */
#define R_CONFIG (0x00 / 4)
diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
index ce1dc63911..96d534d8a8 100644
--- a/hw/timer/a9gtimer.c
+++ b/hw/timer/a9gtimer.c
@@ -37,7 +37,7 @@
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)
diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
index 5e65fdb5a0..10056407ab 100644
--- a/hw/timer/cadence_ttc.c
+++ b/hw/timer/cadence_ttc.c
@@ -24,7 +24,7 @@
#define DB_PRINT(...) do { \
fprintf(stderr, ": %s: ", __func__); \
fprintf(stderr, ## __VA_ARGS__); \
- } while (0);
+ } while (0)
#else
#define DB_PRINT(...)
#endif
diff --git a/hw/timer/mss-timer.c b/hw/timer/mss-timer.c
index 60f1213a3b..4f814572e2 100644
--- a/hw/timer/mss-timer.c
+++ b/hw/timer/mss-timer.c
@@ -36,7 +36,7 @@
if (MSS_TIMER_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt "\n", __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
index e5f5e14a90..58fc7b1188 100644
--- a/hw/timer/stm32f2xx_timer.c
+++ b/hw/timer/stm32f2xx_timer.c
@@ -34,7 +34,7 @@
if (STM_TIMER_ERR_DEBUG >= lvl) { \
qemu_log("%s: " fmt, __func__, ## args); \
} \
-} while (0);
+} while (0)
#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index c440aff4b2..b8253b338d 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -39,7 +39,7 @@
if (DEBUG_TPM) { \
fprintf(stderr, fmt, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define TYPE_TPM_PASSTHROUGH "tpm-passthrough"
#define TPM_PASSTHROUGH(obj) \
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 42d647d363..6e08cc2033 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -105,7 +105,7 @@ struct TPMState {
if (DEBUG_TIS) { \
printf(fmt, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
/* tis registers */
#define TPM_TIS_REG_ACCESS 0x00
diff --git a/migration/rdma.c b/migration/rdma.c
index ca56594328..9d5a424011 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -88,7 +88,7 @@ static uint32_t known_capabilities = RDMA_CAPABILITY_PIN_ALL;
} \
return rdma->error_state; \
} \
- } while (0);
+ } while (0)
/*
* A work request ID is 64-bits and we split up these bits
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 625ef2dfd2..89ffd33e87 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -400,7 +400,7 @@ static void unallocated_encoding(DisasContext *s)
"at pc=%016" PRIx64 "\n", \
__FILE__, __LINE__, insn, s->pc - 4); \
unallocated_encoding(s); \
- } while (0);
+ } while (0)
static void init_tmp_a64_array(DisasContext *s)
{
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index b03f583032..06d06010a4 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -58,7 +58,7 @@
if (DEBUG_KVM) { \
fprintf(stderr, fmt, ## __VA_ARGS__); \
} \
-} while (0);
+} while (0)
#define kvm_vm_check_mem_attr(s, attr) \
kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
diff --git a/tests/tcg/test-mmap.c b/tests/tcg/test-mmap.c
index 3982fa2c72..cdefadfa4c 100644
--- a/tests/tcg/test-mmap.c
+++ b/tests/tcg/test-mmap.c
@@ -39,7 +39,7 @@ do \
fprintf (stderr, "FAILED at %s:%d\n", __FILE__, __LINE__); \
exit (EXIT_FAILURE); \
} \
-} while (0);
+} while (0)
unsigned char *dummybuf;
static unsigned int pagesize;
--
2.14.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
@ 2017-11-30 13:54 ` Cornelia Huck
2017-11-30 14:43 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Cornelia Huck @ 2017-11-30 13:54 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-trivial, Gerd Hoffmann, Alistair Francis,
Peter Crosthwaite, Kevin Wolf, Max Reitz, Edgar E. Iglesias,
Paolo Bonzini, Peter Maydell, Alexander Graf, Jason Wang,
Subbaraya Sundeep, Stefan Berger, Juan Quintela,
Dr. David Alan Gilbert, Christian Borntraeger, Richard Henderson,
Michael S. Tsirkin, Igor Mammedov, open list:Block layer core,
open list:Xilinx Zynq, open list:New World, open list:S390
On Thu, 30 Nov 2017 07:41:59 -0600
Eric Blake <eblake@redhat.com> wrote:
> The point of writing a macro embedded in a 'do { ... } while (0)'
> loop is so that the macro can be used as a drop-in statement with
> the caller supplying the trailing ';'. Although our coding style
> frowns on brace-less 'if':
> if (cond)
> statement;
> else
> something else;
> the use of do/while (0) in a macro is absolutely essential for the
> purpose of avoiding a syntax error on the 'else' - but it only works
> if there is no trailing ';' in the macro (as the ';' in the code
> calling the macro would then be a second statement and cause the
> 'else' to not pair to the 'if').
>
> Many of the places touched in this code are examples of the ugly
> bit-rotting debug print statements; cleaning those up is left as
> a bite-sized task for another day.
>
> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> tests/acpi-utils.h | 8 ++++----
> ui/sdl_zoom_template.h | 8 ++++----
> audio/paaudio.c | 4 ++--
> hw/adc/stm32f2xx_adc.c | 2 +-
> hw/block/m25p80.c | 2 +-
> hw/char/cadence_uart.c | 2 +-
> hw/char/stm32f2xx_usart.c | 2 +-
> hw/display/cg3.c | 2 +-
> hw/display/dpcd.c | 2 +-
> hw/display/xlnx_dp.c | 2 +-
> hw/dma/pl330.c | 2 +-
> hw/dma/xlnx-zynq-devcfg.c | 2 +-
> hw/dma/xlnx_dpdma.c | 2 +-
> hw/i2c/i2c-ddc.c | 2 +-
> hw/misc/auxbus.c | 2 +-
> hw/misc/macio/mac_dbdma.c | 4 ++--
> hw/misc/mmio_interface.c | 2 +-
> hw/misc/stm32f2xx_syscfg.c | 2 +-
> hw/misc/zynq_slcr.c | 2 +-
> hw/net/cadence_gem.c | 2 +-
> hw/ssi/mss-spi.c | 2 +-
> hw/ssi/stm32f2xx_spi.c | 2 +-
> hw/ssi/xilinx_spi.c | 2 +-
> hw/ssi/xilinx_spips.c | 2 +-
> hw/timer/a9gtimer.c | 2 +-
> hw/timer/cadence_ttc.c | 2 +-
> hw/timer/mss-timer.c | 2 +-
> hw/timer/stm32f2xx_timer.c | 2 +-
> hw/tpm/tpm_passthrough.c | 2 +-
> hw/tpm/tpm_tis.c | 2 +-
> migration/rdma.c | 2 +-
> target/arm/translate-a64.c | 2 +-
> target/s390x/kvm.c | 2 +-
> tests/tcg/test-mmap.c | 2 +-
> 34 files changed, 42 insertions(+), 42 deletions(-)
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index b03f583032..06d06010a4 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -58,7 +58,7 @@
> if (DEBUG_KVM) { \
> fprintf(stderr, fmt, ## __VA_ARGS__); \
> } \
> -} while (0);
> +} while (0)
>
> #define kvm_vm_check_mem_attr(s, attr) \
> kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
Acked-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style
2017-11-30 13:41 [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup Eric Blake
` (2 preceding siblings ...)
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
@ 2017-11-30 14:01 ` Eric Blake
2017-11-30 17:00 ` Eric Blake
3 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-11-30 14:01 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial
while (0) is only idiomatic in a macro definition, where the caller
will be supplying the trailing ';'. Warn if the macro has a duplicate.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/checkpatch.pl | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34df753571..acb66bff34 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1622,6 +1622,11 @@ sub process {
}
}
+# 'while (0);' is odd; only macros should use while (0), without trailing ;
+ if ($line =~ /while\s*\(0\);/) {
+ ERROR("suspicious ; after while (0)\n" . $herecurr);
+ }
+
# Check relative indent for conditionals and blocks.
if ($line =~ /\b(?:(?:if|while|for)\s*\(|do\b)/ && $line !~ /^.\s*#/ && $line !~ /\}\s*while\s*/) {
my ($s, $c) = ($stat, $cond);
--
2.14.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
2017-11-30 13:54 ` Cornelia Huck
@ 2017-11-30 14:43 ` Michael S. Tsirkin
2017-11-30 14:55 ` Eric Blake
2017-11-30 14:56 ` Dr. David Alan Gilbert
2017-12-04 0:45 ` [Qemu-devel] [Qemu-ppc] " David Gibson
3 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2017-11-30 14:43 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-trivial, Gerd Hoffmann, Alistair Francis,
Peter Crosthwaite, Kevin Wolf, Max Reitz, Edgar E. Iglesias,
Paolo Bonzini, Peter Maydell, Alexander Graf, Jason Wang,
Subbaraya Sundeep, Stefan Berger, Juan Quintela,
Dr. David Alan Gilbert, Christian Borntraeger, Cornelia Huck,
Richard Henderson, Igor Mammedov, open list:Block layer core,
open list:Xilinx Zynq, open list:New World, open list:S390
On Thu, Nov 30, 2017 at 07:41:59AM -0600, Eric Blake wrote:
> The point of writing a macro embedded in a 'do { ... } while (0)'
> loop is so that the macro can be used as a drop-in statement with
> the caller supplying the trailing ';'. Although our coding style
> frowns on brace-less 'if':
> if (cond)
> statement;
> else
> something else;
> the use of do/while (0) in a macro is absolutely essential for the
> purpose of avoiding a syntax error on the 'else' - but it only works
> if there is no trailing ';' in the macro (as the ';' in the code
> calling the macro would then be a second statement and cause the
> 'else' to not pair to the 'if').
Shouldn't matter if everyone puts the statements in {}, right?
> Many of the places touched in this code are examples of the ugly
> bit-rotting debug print statements; cleaning those up is left as
> a bite-sized task for another day.
>
> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
We can't really rely on code still building for this to do the right
thing, can we? I did my best to look for uses and I think it's OK, so
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
but I'm not merging this.
> --
> 2.14.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 14:43 ` Michael S. Tsirkin
@ 2017-11-30 14:55 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2017-11-30 14:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, qemu-trivial, Gerd Hoffmann, Alistair Francis,
Peter Crosthwaite, Kevin Wolf, Max Reitz, Edgar E. Iglesias,
Paolo Bonzini, Peter Maydell, Alexander Graf, Jason Wang,
Subbaraya Sundeep, Stefan Berger, Juan Quintela,
Dr. David Alan Gilbert, Christian Borntraeger, Cornelia Huck,
Richard Henderson, Igor Mammedov, open list:Block layer core,
open list:Xilinx Zynq, open list:New World, open list:S390
On 11/30/2017 08:43 AM, Michael S. Tsirkin wrote:
> On Thu, Nov 30, 2017 at 07:41:59AM -0600, Eric Blake wrote:
>> The point of writing a macro embedded in a 'do { ... } while (0)'
>> loop is so that the macro can be used as a drop-in statement with
>> the caller supplying the trailing ';'. Although our coding style
>> frowns on brace-less 'if':
>> if (cond)
>> statement;
>> else
>> something else;
>> the use of do/while (0) in a macro is absolutely essential for the
>> purpose of avoiding a syntax error on the 'else' - but it only works
>> if there is no trailing ';' in the macro (as the ';' in the code
>> calling the macro would then be a second statement and cause the
>> 'else' to not pair to the 'if').
>
> Shouldn't matter if everyone puts the statements in {}, right?
Correct - where we follow our style, spurious ';' don't make a
difference (other than they might trigger a warning in a very particular
compiler). But it also makes our code harder to copy-and-paste into
other projects with a different style.
>
>> Many of the places touched in this code are examples of the ugly
>> bit-rotting debug print statements; cleaning those up is left as
>> a bite-sized task for another day.
>>
>> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> We can't really rely on code still building for this to do the right
> thing, can we?
Some of the uses that were changed are in dead #ifdefs for debugging
purposes - the only way to still compile is to turn on the debugging,
but that may fail to compile for other reasons (if the format strings
have bit-rotted, for example).
The only sure way to know that this did not break anything is to audit
that for every macro where I eliminated the ';', all callers of that
macro call 'foo();'. I did not perform that audit (the patch was
mechanical) - but am willing to do so and reply back with results if you
need the extra confidence.
> I did my best to look for uses and I think it's OK, so
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> but I'm not merging this.
What tree should it go through instead? Does it need to be split along
maintainer boundaries?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
2017-11-30 13:54 ` Cornelia Huck
2017-11-30 14:43 ` Michael S. Tsirkin
@ 2017-11-30 14:56 ` Dr. David Alan Gilbert
2017-12-04 0:45 ` [Qemu-devel] [Qemu-ppc] " David Gibson
3 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2017-11-30 14:56 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-trivial, Gerd Hoffmann, Alistair Francis,
Peter Crosthwaite, Kevin Wolf, Max Reitz, Edgar E. Iglesias,
Paolo Bonzini, Peter Maydell, Alexander Graf, Jason Wang,
Subbaraya Sundeep, Stefan Berger, Juan Quintela,
Christian Borntraeger, Cornelia Huck, Richard Henderson,
Michael S. Tsirkin, Igor Mammedov, open list:Block layer core,
open list:Xilinx Zynq, open list:New World, open list:S390
* Eric Blake (eblake@redhat.com) wrote:
> The point of writing a macro embedded in a 'do { ... } while (0)'
> loop is so that the macro can be used as a drop-in statement with
> the caller supplying the trailing ';'. Although our coding style
> frowns on brace-less 'if':
> if (cond)
> statement;
> else
> something else;
> the use of do/while (0) in a macro is absolutely essential for the
> purpose of avoiding a syntax error on the 'else' - but it only works
> if there is no trailing ';' in the macro (as the ';' in the code
> calling the macro would then be a second statement and cause the
> 'else' to not pair to the 'if').
>
> Many of the places touched in this code are examples of the ugly
> bit-rotting debug print statements; cleaning those up is left as
> a bite-sized task for another day.
>
> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> diff --git a/migration/rdma.c b/migration/rdma.c
> index ca56594328..9d5a424011 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -88,7 +88,7 @@ static uint32_t known_capabilities = RDMA_CAPABILITY_PIN_ALL;
> } \
> return rdma->error_state; \
> } \
> - } while (0);
> + } while (0)
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros
2017-11-30 13:41 ` [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros Eric Blake
@ 2017-11-30 15:33 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-11-30 15:33 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: qemu-trivial, Yongbok Kim, Aurelien Jarno
On 11/30/2017 10:41 AM, Eric Blake wrote:
> It is more typical to provide the ';' by the caller of a macro
> than to embed it in the macro itself; this is because syntax
> highlight engines can get confused if a macro is called without
> a semicolon before the closing '}'.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> target/mips/msa_helper.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
> index f167a42655..8fb7a369ca 100644
> --- a/target/mips/msa_helper.c
> +++ b/target/mips/msa_helper.c
> @@ -682,13 +682,13 @@ static inline int64_t msa_mod_u_df(uint32_t df, int64_t arg1, int64_t arg2)
> do { \
> e = SIGNED_EVEN(a, df); \
> o = SIGNED_ODD(a, df); \
> - } while (0);
> + } while (0)
>
> #define UNSIGNED_EXTRACT(e, o, a, df) \
> do { \
> e = UNSIGNED_EVEN(a, df); \
> o = UNSIGNED_ODD(a, df); \
> - } while (0);
> + } while (0)
>
> static inline int64_t msa_dotp_s_df(uint32_t df, int64_t arg1, int64_t arg2)
> {
> @@ -1120,9 +1120,11 @@ void helper_msa_splat_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
> #define MSA_LOOP_COND_D MSA_LOOP_COND(DF_DOUBLE)
>
> #define MSA_LOOP(DF) \
> + do { \
> for (i = 0; i < (MSA_LOOP_COND_ ## DF) ; i++) { \
> - MSA_DO_ ## DF \
> - }
> + MSA_DO_ ## DF; \
> + } \
> + } while (0)
>
> #define MSA_FN_DF(FUNC) \
> void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
> @@ -1135,17 +1137,17 @@ void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
> uint32_t i; \
> switch (df) { \
> case DF_BYTE: \
> - MSA_LOOP_B \
> + MSA_LOOP_B; \
> break; \
> case DF_HALF: \
> - MSA_LOOP_H \
> + MSA_LOOP_H; \
> break; \
> case DF_WORD: \
> - MSA_LOOP_W \
> + MSA_LOOP_W; \
> break; \
> case DF_DOUBLE: \
> - MSA_LOOP_D \
> - break; \
> + MSA_LOOP_D; \
> + break; \
> default: \
> assert(0); \
> } \
> @@ -1168,7 +1170,7 @@ void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
> do { \
> R##DF(pwx, i) = pwt->DF[2*i]; \
> L##DF(pwx, i) = pws->DF[2*i]; \
> - } while (0);
> + } while (0)
> MSA_FN_DF(pckev_df)
> #undef MSA_DO
>
> @@ -1176,7 +1178,7 @@ MSA_FN_DF(pckev_df)
> do { \
> R##DF(pwx, i) = pwt->DF[2*i+1]; \
> L##DF(pwx, i) = pws->DF[2*i+1]; \
> - } while (0);
> + } while (0)
> MSA_FN_DF(pckod_df)
> #undef MSA_DO
>
> @@ -1184,7 +1186,7 @@ MSA_FN_DF(pckod_df)
> do { \
> pwx->DF[2*i] = L##DF(pwt, i); \
> pwx->DF[2*i+1] = L##DF(pws, i); \
> - } while (0);
> + } while (0)
> MSA_FN_DF(ilvl_df)
> #undef MSA_DO
>
> @@ -1192,7 +1194,7 @@ MSA_FN_DF(ilvl_df)
> do { \
> pwx->DF[2*i] = R##DF(pwt, i); \
> pwx->DF[2*i+1] = R##DF(pws, i); \
> - } while (0);
> + } while (0)
> MSA_FN_DF(ilvr_df)
> #undef MSA_DO
>
> @@ -1200,7 +1202,7 @@ MSA_FN_DF(ilvr_df)
> do { \
> pwx->DF[2*i] = pwt->DF[2*i]; \
> pwx->DF[2*i+1] = pws->DF[2*i]; \
> - } while (0);
> + } while (0)
> MSA_FN_DF(ilvev_df)
> #undef MSA_DO
>
> @@ -1208,7 +1210,7 @@ MSA_FN_DF(ilvev_df)
> do { \
> pwx->DF[2*i] = pwt->DF[2*i+1]; \
> pwx->DF[2*i+1] = pws->DF[2*i+1]; \
> - } while (0);
> + } while (0)
> MSA_FN_DF(ilvod_df)
> #undef MSA_DO
> #undef MSA_LOOP_COND
> @@ -1222,7 +1224,7 @@ MSA_FN_DF(ilvod_df)
> uint32_t k = (pwd->DF[i] & 0x3f) % (2 * n); \
> pwx->DF[i] = \
> (pwd->DF[i] & 0xc0) ? 0 : k < n ? pwt->DF[k] : pws->DF[k - n]; \
> - } while (0);
> + } while (0)
> MSA_FN_DF(vshf_df)
> #undef MSA_DO
> #undef MSA_LOOP_COND
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0);
2017-11-30 13:41 ` [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0); Eric Blake
@ 2017-11-30 16:08 ` Thomas Huth
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Huth @ 2017-11-30 16:08 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: qemu-trivial, Jason Wang
On 30.11.2017 14:41, Eric Blake wrote:
> For a couple of macros in pcnet.c, we have to provide a new scope
> to avoid compiler warnings about declarations in the middle of a
> switch statement that aren't in a sub-scope. But use of
> 'do { ... } while (0);' merely to provide that new scope is arcane
> overkill, compared to just using '{ ... }'.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> hw/net/pcnet.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style
2017-11-30 14:01 ` [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style Eric Blake
@ 2017-11-30 17:00 ` Eric Blake
2017-12-01 7:31 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-11-30 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial
On 11/30/2017 08:01 AM, Eric Blake wrote:
> while (0) is only idiomatic in a macro definition, where the caller
> will be supplying the trailing ';'. Warn if the macro has a duplicate.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> scripts/checkpatch.pl | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 34df753571..acb66bff34 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1622,6 +1622,11 @@ sub process {
> }
> }
>
> +# 'while (0);' is odd; only macros should use while (0), without trailing ;
> + if ($line =~ /while\s*\(0\);/) {
Should this also check for uses of 'while (false);' ?
Interestingly enough, we have an instance of 'do/while (false);' in
tests/vhost-user-bridge.c that is NOT in a macro, but is used for the
convenience of being able to 'break;' out early rather than using a
goto. Similarly for chardev/char-serial.c using 'while (0);' outside of
a macro. Those may be worth rewriting to use goto as separate patches
if we want to restrict ALL use of 'while \((0|false)\);'
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style
2017-11-30 17:00 ` Eric Blake
@ 2017-12-01 7:31 ` Markus Armbruster
2017-12-01 14:22 ` Eric Blake
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2017-12-01 7:31 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-trivial
Eric Blake <eblake@redhat.com> writes:
> On 11/30/2017 08:01 AM, Eric Blake wrote:
>> while (0) is only idiomatic in a macro definition, where the caller
>> will be supplying the trailing ';'. Warn if the macro has a duplicate.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> scripts/checkpatch.pl | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 34df753571..acb66bff34 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1622,6 +1622,11 @@ sub process {
>> }
>> }
>>
>> +# 'while (0);' is odd; only macros should use while (0), without trailing ;
>> + if ($line =~ /while\s*\(0\);/) {
>
> Should this also check for uses of 'while (false);' ?
Do we think it's likely to occur?
> Interestingly enough, we have an instance of 'do/while (false);' in
> tests/vhost-user-bridge.c that is NOT in a macro, but is used for the
> convenience of being able to 'break;' out early rather than using a
> goto. Similarly for chardev/char-serial.c using 'while (0);' outside
> of a macro.
That "cure" merely adds gratuitous cleverness to the "disease".
> Those may be worth rewriting to use goto as separate
> patches if we want to restrict ALL use of 'while \((0|false)\);'
I'd support that.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style
2017-12-01 7:31 ` Markus Armbruster
@ 2017-12-01 14:22 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2017-12-01 14:22 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-trivial
On 12/01/2017 01:31 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 11/30/2017 08:01 AM, Eric Blake wrote:
>>> while (0) is only idiomatic in a macro definition, where the caller
>>> will be supplying the trailing ';'. Warn if the macro has a duplicate.
>>>
>>>
>>> +# 'while (0);' is odd; only macros should use while (0), without trailing ;
>>> + if ($line =~ /while\s*\(0\);/) {
>>
>> Should this also check for uses of 'while (false);' ?
>
> Do we think it's likely to occur?
Not as frequent, but it does appear in the code base.
>
>> Interestingly enough, we have an instance of 'do/while (false);' in
>> tests/vhost-user-bridge.c that is NOT in a macro, but is used for the
>> convenience of being able to 'break;' out early rather than using a
>> goto. Similarly for chardev/char-serial.c using 'while (0);' outside
>> of a macro.
>
> That "cure" merely adds gratuitous cleverness to the "disease".
>
>> Those may be worth rewriting to use goto as separate
>> patches if we want to restrict ALL use of 'while \((0|false)\);'
>
> I'd support that.
>
Okay, I'll post a v2 along those lines.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
` (2 preceding siblings ...)
2017-11-30 14:56 ` Dr. David Alan Gilbert
@ 2017-12-04 0:45 ` David Gibson
3 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2017-12-04 0:45 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Peter Maydell, Michael S. Tsirkin, Jason Wang,
Juan Quintela, Gerd Hoffmann, Subbaraya Sundeep,
open list:Block layer core, Stefan Berger, qemu-trivial,
Christian Borntraeger, Alistair Francis, Dr. David Alan Gilbert,
open list:S390, open list:Xilinx Zynq, Paolo Bonzini,
Richard Henderson, Kevin Wolf, Peter Crosthwaite, Cornelia Huck,
Max Reitz, open list:New World, Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]
On Thu, Nov 30, 2017 at 07:41:59AM -0600, Eric Blake wrote:
> The point of writing a macro embedded in a 'do { ... } while (0)'
> loop is so that the macro can be used as a drop-in statement with
> the caller supplying the trailing ';'. Although our coding style
> frowns on brace-less 'if':
> if (cond)
> statement;
> else
> something else;
> the use of do/while (0) in a macro is absolutely essential for the
> purpose of avoiding a syntax error on the 'else' - but it only works
> if there is no trailing ';' in the macro (as the ';' in the code
> calling the macro would then be a second statement and cause the
> 'else' to not pair to the 'if').
>
> Many of the places touched in this code are examples of the ugly
> bit-rotting debug print statements; cleaning those up is left as
> a bite-sized task for another day.
>
> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1 \\\\
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
ppc related portions
Acked-by: David Gibson <david@gibson.dropbear.id.au>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-12-04 1:08 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-30 13:41 [Qemu-devel] [PATCH 0/3] macro do/while (0) cleanup Eric Blake
2017-11-30 13:41 ` [Qemu-devel] [PATCH 1/3] net: Drop unusual use of do { } while (0); Eric Blake
2017-11-30 16:08 ` Thomas Huth
2017-11-30 13:41 ` [Qemu-devel] [PATCH 2/3] mips: Tweak location of ';' in macros Eric Blake
2017-11-30 15:33 ` Philippe Mathieu-Daudé
2017-11-30 13:41 ` [Qemu-devel] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage Eric Blake
2017-11-30 13:54 ` Cornelia Huck
2017-11-30 14:43 ` Michael S. Tsirkin
2017-11-30 14:55 ` Eric Blake
2017-11-30 14:56 ` Dr. David Alan Gilbert
2017-12-04 0:45 ` [Qemu-devel] [Qemu-ppc] " David Gibson
2017-11-30 14:01 ` [Qemu-devel] [PATCH 4/3] checkpatch: Enforce proper do/while (0) style Eric Blake
2017-11-30 17:00 ` Eric Blake
2017-12-01 7:31 ` Markus Armbruster
2017-12-01 14:22 ` Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).