This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a namespace checker.
ClosedPublic

Authored by bkramer on Jul 15 2014, 9:58 AM.

Details

Summary

This warns on two constructs.

  1. anonymous namespaces in header files.
  2. using namespace directives everywhere.

Diff Detail

Repository
rL LLVM

Event Timeline

bkramer updated this revision to Diff 11452.Jul 15 2014, 9:58 AM
bkramer retitled this revision from to [clang-tidy] Add a namespace checker..
bkramer updated this object.
bkramer added reviewers: alexfh, djasper.
bkramer added a subscriber: Unknown Object (MLST).
bkramer updated this revision to Diff 11455.Jul 15 2014, 10:29 AM

Remove test that should've been part of D4522.

djasper added inline comments.Jul 16 2014, 2:11 AM
clang-tidy/google/GoogleTidyModule.cpp
32–33 ↗(On Diff #11455)

I think both the check name and the class Name should be more specific, e.g. UnnamedNamespaceInHeaderCheck. Or do you envision this to be a more generic check?

clang-tidy/google/NamespacesCheck.cpp
27 ↗(On Diff #11455)

nit: I'd probably remove the empty lines inside the ast_matchers namespace but add an empty line before starting the tidy namespace.

clang-tidy/google/NamespacesCheck.h
19 ↗(On Diff #11455)

IMO, "using namespace directives" should be detected in a separate check, there doesn't seem to be a strong relation to the empty namespaces in headers.

20 ↗(On Diff #11455)

I actually don't think we should keep references to a different tool. The meaning of "build/namespaces" might change making this comment stale. This check is good on its own without a specific reference.

alexfh added inline comments.Jul 16 2014, 2:48 AM
clang-tidy/google/NamespacesCheck.h
19 ↗(On Diff #11455)

nit: Add an empty line after the \brief line?

20 ↗(On Diff #11455)

The main use of this is if we'll want to support categorized NOLINT comments: "// NOLINT(build/namespaces)", then it would be nice to have them in the same form.

alexfh added inline comments.Jul 16 2014, 4:01 AM
clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
38 ↗(On Diff #11495)

nit: I'd use early return instead.

41 ↗(On Diff #11495)

The list of header file extensions may be a good candidate for being stored in ClangTidyOptions, so we can configure it when needed.

clang-tidy/google/UsingNamespaceDirectiveCheck.cpp
30 ↗(On Diff #11495)

nit: Use early exit instead? (Especially, if we're going to implement fix-its)

31 ↗(On Diff #11495)

Further down the line we may want to suggest the list of using declarations as a replacement. Could you add a TODO?

bkramer added inline comments.Jul 16 2014, 4:36 AM
clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
41 ↗(On Diff #11495)

Hmm, we don't have a way of accessing ClangTidyOptions from within a checker?

alexfh added inline comments.Jul 16 2014, 5:19 AM
clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
41 ↗(On Diff #11495)

Not yet. I meant, that we should do this in general, not necessarily now. Sorry for ambiguous language.

bkramer updated this revision to Diff 11500.Jul 16 2014, 5:56 AM

Use early exits, add TODOs.

alexfh accepted this revision.Jul 16 2014, 6:02 AM
alexfh edited edge metadata.

Looks good. Thanks!

This revision is now accepted and ready to land.Jul 16 2014, 6:02 AM
bkramer closed this revision.Jul 16 2014, 7:25 AM
bkramer updated this revision to Diff 11510.

Closed by commit rL213153 (authored by d0k).