Statement macros should be wrapped in do{}while(0)

Description

Certain statement macros like U16_APPEND et al are defined to expand to the form "{ ... }". They should instead expand to the common C idiom of "do { ... } while (0)", such that a semi-colon after them is not deemed redundant by the compiler and they'd work fine in an if-else block. Right now at HarfBuzz we are getting these error from compiler:

hb-icu.cc:261:39: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
U16_GET_UNSAFE (normalized, 0, *a);
^
hb-icu.cc:266:42: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
U16_NEXT_UNSAFE (normalized, len, *a);
^
hb-icu.cc:267:42: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
U16_NEXT_UNSAFE (normalized, len, *b);
^

Activity

Show:
Markus Scherer
May 22, 2019, 10:04 PM
Edited

Discussed in the team meeting today.

if-else, loops etc. should always use curly braces, and with that there is no functional problem.

However, it makes some sense to help projects that want to work with -Wextra-semi-stmt.

We have long said that these macros should be used with a terminating semicolon.

Problem: Code without that semicolon used to compile fine, and if we change the macros so that the semicolon is required, some callers may break. Easy to fix but still…

Suggestion: We should be able to make one new @internal macro like

and use that at the start other public ICU macros to avoid the extra-semi-stmt warning.

If someone has a lot of code that breaks, then they could make a one-line patch to make this new macro empty.

Behdad Esfahbod
May 23, 2019, 3:53 PM

Interesting. I didn’t know about switch(0) default:. Does a semicolon after a block after that deemed not extra?

Fredrik Roubert
August 15, 2019, 1:10 PM
Edited

Unfortunately, it turns out that not all compound macros in the public ICU API that currently cause -Wextra-semi-stmt are simple if-then-else statements, so solving this won’t be as easy as just inserting some switch statements in the right places.

Fortunately, these macros are already wrapped in { } blocks, which makes it possible to both do the right thing going forward while still providing a simple compile-time command line flag to get the old behaviour back, by using the standard do { } while construct for creating macros that behave like statements, but allowing conditional compilation of it.

With two macro definitions like this:

… the macros that currently are surrounded by { } could just be updated to UPRV_BLOCK_MACRO_BEGIN { } UPRV_BLOCK_MACRO_END instead and suddenly they will start behave like proper statements and not cause -Wextra-semi-stmt when followed by a semicolon, but the old behavior would still be only a preprocessor define override away.

Markus Scherer
August 15, 2019, 8:22 PM

Fredrik: Could you please try to add the -Wextra-semi-stmt flag to one of the build bots?

Fredrik Roubert
August 15, 2019, 8:24 PM

No, unfortunately, as far as I’ve been able to find out, we don’t have any build bot with a compiler that supports the -Wextra-semi-stmt flag.

Fixed

Assignee

Fredrik Roubert

Reporter

Behdad Esfahbod

Components

Labels

None

Reviewer

Markus Scherer

Priority

minor

Time Needed

Hours

Fix versions

Configure