This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116)

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[0] 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
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c

+  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
clause token.

Likewise; is "conditional" the right word here?  Also, whether of not
the statements are actually "in" the body of the guard is the issue

How about:

"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."

or somesuch?

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.)

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]