This is the mail archive of the
mailing list for the GCC project.
Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116)
- From: Martin Sebor <msebor at gmail dot com>
- To: David Malcolm <dmalcolm at redhat dot com>, Marek Polacek <polacek at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Joseph Myers <joseph at codesourcery dot com>, Martin Sebor <msebor at redhat dot com>, Jason Merrill <jason at redhat dot com>
- Date: Thu, 8 Jun 2017 12:10:38 -0600
- Subject: Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116)
- Authentication-results: sourceware.org; auth=none
- References: <20170608164936.GV3413@redhat.com> <email@example.com>
On 06/08/2017 11:24 AM, David Malcolm wrote:
On Thu, 2017-06-08 at 18:49 +0200, Marek Polacek wrote:
This is the hopefully last incarnation of the patch. The change from
last time is simpy that I've added a new test and the warning has
renamed to -Wmultistatement-macros.
David - any another comments?
Thanks for working on this; looks useful.
The new name is more accurate, but is rather long; oh well. As part of
-Wall, users won't typically have to type it, so that's OK.
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 35321a6..d883330 100644
+ if (warning_at (body_loc, OPT_Wmultistatement_macros,
+ "macro expands to multiple statements"))
+ inform (guard_loc, "some parts of macro expansion are not
guarded by "
+ "this conditional");
Is the guard necessarily a "conditional"? I take a "conditional" to
mean an "if"; the guard could be a "for" or a "while" (or an "else",
which still seems something of a stretch to me to call a
Suggestion: word "this conditional" as "this %qs clause" and either (a)
rework the code in c-indentation.c's guard_tinfo_to_string so that it's
shared between these two warnings (i.e. to go from a RID_ to a const
char *), or (b) just pass in a const char * identifying the guard
Likewise; is "conditional" the right word here? Also, whether of not
the statements are actually "in" the body of the guard is the issue
"Warn about unsafe multiple statement macros that appear to be guarded
by a clause such as if, else, while, or for, in which only the first
statement is actually guarded after the macro is expanded."
FWIW, I agree with David that "conditional" isn't entirely accurate.
At the same time, referring to any of do, for, if, or switch as
clauses isn't quite precise either(*). In the C language they are
the names of selection and iteration statements, and what follows
is called the controlling expression (with for being special) and
the next thing is a substatement. I think many people will
informally call the two a condition or conditional and the body.
I don't have strong feelings about the current wording but if it
should be tweaked for accuracy I would suggest to use the formal
term "controlling expression", similarly to -Wswitch-unreachable.
PS [*] To be completely pedantic, the word clause in the C and
C++ standards has a precise meaning: it refers to a chapter of
the text (such as Scope, Conformance, Language, etc.)