Page MenuHomePhabricator

[clang-tidy] Add check misc-multiple-statement-macro
ClosedPublic

Authored by sbenza on Apr 4 2016, 11:27 AM.

Details

Summary

The check detects multi-statement macros that are used in unbraced conditionals.
Only the first statement will be part of the conditionals and the rest will fall
outside of it and executed unconditionally.

Diff Detail

Repository
rL LLVM

Event Timeline

sbenza updated this revision to Diff 52585.Apr 4 2016, 11:27 AM
sbenza retitled this revision from to [clang-tidy] Add check misc-multiple-statement-macro.
sbenza updated this object.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.

this is cool.

clang-tidy/misc/MultipleStatementMacroCheck.cpp
33 ↗(On Diff #52585)

I feel it nicer if you merge this namespace with the one at line 20.

42 ↗(On Diff #52585)

Why not this?

if (found)
  return Child;
else
  found = Child == S;    // why the "|" is needed?
75 ↗(On Diff #52585)

nit: no { }

sbenza updated this revision to Diff 52625.Apr 4 2016, 2:18 PM
sbenza marked an inline comment as done.

Minor fixes

sbenza marked an inline comment as done and an inline comment as not done.Apr 4 2016, 2:18 PM
sbenza added inline comments.
clang-tidy/misc/MultipleStatementMacroCheck.cpp
33 ↗(On Diff #52585)

I like to put the helper functions closer to where they are used.
Merging the namespaces means I have to put everything at the top.

42 ↗(On Diff #52585)

Fixed a different way.

thanks, lgtm.

hokein edited edge metadata.Apr 5 2016, 3:12 AM

Nice check! I like the check, it would be very helpful. (I have struggled with this kind of bug before)

clang-tidy/misc/MultipleStatementMacroCheck.cpp
33 ↗(On Diff #52625)

I think you can put this anonymous namespace into the above one.

37 ↗(On Diff #52625)

Should we check the result returned by Result.Context->getParents is valid or not?

docs/clang-tidy/checks/misc-multiple-statement-macro.rst
15 ↗(On Diff #52625)

Would be better to add a comment to explain the sample.

sbenza updated this revision to Diff 52702.Apr 5 2016, 9:06 AM
sbenza marked 2 inline comments as done and an inline comment as not done.
sbenza edited edge metadata.

Minor fixes

docs/clang-tidy/checks/misc-multiple-statement-macro.rst
15 ↗(On Diff #52625)

The sentence just before the example explains what the problem is.

alexfh accepted this revision.Apr 7 2016, 9:15 AM
alexfh edited edge metadata.

LG with a nit.

clang-tidy/misc/MultipleStatementMacroCheck.cpp
99 ↗(On Diff #52702)

If I saw this message from a tool, I wouldn't get what it means right away. Can you come up with an easier to read alternative? I can only suggest multiple statement macro used without braces, but maybe a more self-documenting message comes to your mind.

docs/clang-tidy/checks/misc-multiple-statement-macro.rst
16 ↗(On Diff #52702)

While I see your point, this doesn't seem worse to me:

if (do_increment)
  INCREMENT_TWO(a, b); // `(b)++;` will always be executed.

Up to you though.

This revision is now accepted and ready to land.Apr 7 2016, 9:15 AM
sbenza updated this revision to Diff 53398.Apr 12 2016, 8:13 AM
sbenza marked an inline comment as done.
sbenza edited edge metadata.

Change warning message.

clang-tidy/misc/MultipleStatementMacroCheck.cpp
99 ↗(On Diff #52702)

Sure.
I wrote my methodology to find the problem, not the actual problem.
What about this?

alexfh added inline comments.Apr 12 2016, 9:10 AM
clang-tidy/misc/MultipleStatementMacroCheck.cpp
100 ↗(On Diff #53398)

Now it seems clear to me. Thanks!

This revision was automatically updated to reflect the committed changes.