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 -Wmultiline-expansion (PR c/80116)

On 06/01/2017 10:45 AM, Marek Polacek wrote:
A motivating example for this warning can be found e.g. in

  PRE10-C. Wrap multistatement macros in a do-while loop


#define SWAP(x, y) \
  tmp = x; \
  x = y; \
  y = tmp

used like this [1]

int x, y, z, tmp;
if (z == 0)
  SWAP(x, y);

expands to the following [2], which is certainly not what the programmer intended:

int x, y, z, tmp;
if (z == 0)
  tmp = x;
x = y;
y = tmp;

This has also happened in our codebase, see PR80063.

I tried to summarize the way I approached this problem in the commentary in
warn_for_multiline_expansion, but I'll try to explain the crux of the matter
here, too.

For code like [1], in the FEs we'll see [2], of course.  When parsing the
then-branch we see that the body of the if isn't wrapped in { } so we create a
compound statement with just the first statement "tmp = x;", and the other two
will be executed unconditionally.

My idea was to look at the location info of the following token after the body
of the if has been parsed and determine if they come from the same macro expansion,
and if they do (and the if itself doesn't), warn (taking into account various
corner cases, as usually).

Very nice.  I think David already suggested handling other statements
besides if (do/while), so let me just add for and switch (as in:
'switch (1) case SWAP (i, j);')

The location in the warning look like it could be improved to extend
from just the first column to the whole macro argument but I don't
suppose that's under the direct control of your patch.

Besides the statements already mentioned above, here are a couple
of corner cases I noticed are not handled while playing with the

  define M(x) x

  int f (int i)
    if (i)
      M (--i; --i);   // can this be handled?

    return i;


  define M(x) x; x

  int f (int i)
    if (i)
      M (--i; --i);   // seems like this should be handled

    return i;

As an aside since it's outside the subset of the bigger problem
you chose to solve, there is a related issue with macros that
expand to an unparenthesized binary (and even some unary)

  #define sum(x, y) x + y

  int n = 2 * sum (3, 5);

I'm not very familiar with this area of the parser but I would
expect it to be relatively straightforward to extend your solution
to handle this problem as well.

For this I had to dive into line_maps, macro maps, etc., so CCing David to check
if my understanding of that is reasonable (hadn't worked with them before).

I've included this warning in -Wall, because there should be no false positives
(fingers crossed) and for most cases the warning should be pretty cheap.

I probably should've added a fix-it hint for good measure, too ("you better wrap
the damn macro in do {} while (0)"), but that can be done as a follow-up.

A hint I'm sure would be helpful to a lot of users.  One caveat
to be aware of is that wrapping an expression in a 'do { } while
(0)' is not a viable solution when the value of the last statement
is used.  In those cases, using the comma expression instead (in
parentheses) is often the way to go.  I'd expect determining which
to offer to be less than trivial.


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