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
