On 06/02/2017 10:52 AM, Marek Polacek wrote:
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 same way as 'if (1) SWAP (i, j);' because it expands to
switch (1) case 1: tmp = i;
i = j;
j = tmp;
(I had a typo there so maybe that obscured the problem.)
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;
^
Yes.
? 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.
Yes, it would be if the warning was only meant to trigger for
uses that change the meaning. If it was meant to warn on unsafe
macro definitions instead it should be less so. But that would
make it a different warning, and mean implementing it in the
preprocessor. Hmm, I guess it's not as straightforward as it
seemed.
Martin
