[PATCH] D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:22
+  // The target using declaration is either:
+  // 1. not in any namespace declaration, or
+  // 2. in some namespace declaration but not in the innermost layer
----------------
Why is this not also checking that the using declaration is not within a header file?


================
Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:24-27
+  Finder->addMatcher(usingDecl(eachOf(
+  	usingDecl(unless(hasParent(namespaceDecl()))),
+    usingDecl(hasParent(namespaceDecl(forEach(namespaceDecl())))) )
+    ).bind("use"), this);
----------------
Did clang-format produce this? If not,  you should run the patch through clang-format.


================
Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:32
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UsingDecl>("use");
+  diag(MatchedDecl->getLocation(), "UsingDecl %0 should be in the innermost "
+    "scope. See: https://abseil.io/tips/119")
----------------
aaron.ballman wrote:
> Do not add links to diagnostic messages. They should stand on their own, and not be grammatical with punctuation. I would suggest: `using declaration not declared in the innermost namespace`
I suspect this diagnostic is going to be too aggressive and once you add more tests will find it needs some additional constraints.


================
Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:32-33
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UsingDecl>("use");
+  diag(MatchedDecl->getLocation(), "UsingDecl %0 should be in the innermost "
+    "scope. See: https://abseil.io/tips/119")
+      << MatchedDecl;
----------------
Do not add links to diagnostic messages. They should stand on their own, and not be grammatical with punctuation. I would suggest: `using declaration not declared in the innermost namespace`


================
Comment at: test/clang-tidy/abseil-safely-scoped.cpp:1
+// RUN: %check_clang_tidy %s abseil-safely-scoped %t
+namespace bar {
----------------
You're missing a lot of tests, such as using declarations at function scope, within a class scope, etc.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55411/new/

https://reviews.llvm.org/D55411