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