This is the mail archive of the gcc-patches@gcc.gnu.org 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 Thu, Jun 01, 2017 at 04:17:24PM -0600, Martin Sebor wrote:
> 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);')

How's that one problematic, though?

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

Well, for e.g. the "in expasion" message we have a good location range:
  SWAP
  ^~~~
so do you mean this?
tmp = x;
^
?  But yea, it's outside the scope of this patch.

> Besides the statements already mentioned above, here are a couple
> of corner cases I noticed are not handled while playing with the
> patch:
> 
>   define M(x) x
> 
>   int f (int i)
>   {
>     if (i)
>       M (--i; --i);   // can this be handled?
> 
>     return i;
>   }
> 
> and
> 
>   define M(x) x; x
> 
>   int f (int i)
>   {
>     if (i)
>       M (--i; --i);   // seems like this should be handled
> 
>     return i;
>   }

Hmm, I was hoping my patch would warn for both examples, but it doesn't.  I'll
have to get back to this a ponder more.

> 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)
> expression:
> 
>   #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.

It'd certainly be useful to warn here.  But it doesn't seem to be an
easy warning to implement, to me.  E.g. warning for

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

would be annoying, I suspect.

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

This didn't occur to me at all.  Well, I still think we could just suggest
adding "do while (0)" and get away with that.

Thanks for taking a look!

	Marek


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