This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a check to detect static definitions in anonymous namespace.
ClosedPublic

Authored by hokein on Mar 15 2016, 5:07 AM.

Diff Detail

Event Timeline

hokein updated this revision to Diff 50715.Mar 15 2016, 5:07 AM
hokein retitled this revision from to [clang-tidy] Add a check to detect static definitions in anonymous namespace..
hokein updated this object.
hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
alexfh edited edge metadata.Apr 1 2016, 7:36 AM

Sorry for the delay. A few minor issues.

clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
47

I'd expand the message a bit: "; static is redundant here".

docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
9

nit: in _the_ current _translation_ unit.

test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
2

Please add a couple of tests for different cases with macros.

10

This check is not helpful, it will match static int c = 1; as well, since it's not anchored to the line start. Should be:
CHECK-FIXES: {{^}}int c = 1;. Same in other checks.

alexfh requested changes to this revision.Apr 1 2016, 7:36 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 1 2016, 7:36 AM
hokein updated this revision to Diff 52532.Apr 4 2016, 1:53 AM
hokein edited edge metadata.
hokein marked an inline comment as done.

Address comments.

hokein updated this revision to Diff 52533.Apr 4 2016, 1:54 AM
hokein marked 3 inline comments as done.
hokein edited edge metadata.

Remove dump code.

hokein updated this revision to Diff 52534.Apr 4 2016, 1:55 AM

doc update.

alexfh added inline comments.Apr 4 2016, 4:53 AM
clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
53

DiagnosticBuilder handles hella different stuff. I wonder whether this code works without ->getName()?

docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
10

Still reads a bit strange. A bit more wordsmithing:

In this case, `static` is redundant, because anonymous namespace limits the visibility of definitions to a single translation unit.
test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
33

Please add a CHECK-FIXES for this case as well. Also move the macro definition closer to its usage and add a CHECK-FIXES to ensure it doesn't get changed.

Same below.

alexfh requested changes to this revision.Apr 4 2016, 4:53 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 4 2016, 4:53 AM
hokein updated this revision to Diff 52552.Apr 4 2016, 7:21 AM
hokein edited edge metadata.
hokein marked 3 inline comments as done.

Address comments.

clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
53

Using Def works!

alexfh added inline comments.Apr 4 2016, 8:19 AM
clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
50

nit: auto?

docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
9

Double backquotes should be used for inline code snippets (yeah, I know, this is confusing: different flavors of markdown and RST are completely inconsistent in this regard).

test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
34

You missed the "Also move the macro definition closer to its usage and add a CHECK-FIXES to ensure it doesn't get changed." part.

Same below.

hokein updated this revision to Diff 52661.Apr 5 2016, 12:46 AM
hokein edited edge metadata.

Update

hokein marked 2 inline comments as done.Apr 5 2016, 12:47 AM
hokein added inline comments.
test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
35

Oops. I misunderstood your comment. Done now.

alexfh added inline comments.Apr 5 2016, 12:53 AM
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
10

This is still not done.

test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
35

Sorry for being unclear again: please add

// CHECK-FIXES: #define DEFINE_STATIC_VAR(x) static int x = 2

to verify that the check doesn't unintentionally remove static from the macro definition. Same for the macro above.

hokein updated this revision to Diff 52663.Apr 5 2016, 1:13 AM
hokein marked 2 inline comments as done.
  • fix inline code snippet in rst.
  • Add check-message for macro in test code.
alexfh accepted this revision.Apr 5 2016, 1:15 AM
alexfh edited edge metadata.

Awesome. Thank you!
LG

This revision is now accepted and ready to land.Apr 5 2016, 1:15 AM
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.