[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
----------------
aaron.ballman wrote:
> Why is this not also checking that the using declaration is not within a header file?
I'm still wondering about this. The Abseil tip specifically says `Never declare namespace aliases or convenience using-declarations at namespace scope in header files, only in .cc files.`, which is why I had figured I would see some enforcement from this check.


================
Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:37
+  diag(MatchedDecl->getLocation(),
+       "using declaration %0 not declared in the innermost namespace.")
+      << MatchedDecl;
----------------
aaron.ballman wrote:
> You should remove the full stop at the end of the diagnostic.
The diagnostic still doesn't meet our (somewhat strange) requirements; diagnostics should not be full sentences with capitalization and punctuation. However, it also doesn't really tell the user what is wrong with their code, just how to fix it to make the diagnostic go away.

I don't have a particularly great suggestion for a replacement, however. I'm still trying to wrap my mind around the suggested changes to code, because it seems like this advice isn't general purpose. Consider:
```
namespace Foo { // I control this namespace.
  namespace details {
    int func(int I);
  } // namespace details

  using Frobble::Bobble;

  void f(SomeNameFromFrobbleBobble FB) {
    FB.whatever(details::func(12));
  }
} // namespace Foo
```
The suggestion to move the using declaration into the innermost namespace is actually a poor one -- we don't want the interface of `f()` to be `void f(details::SomeNameFromFrobbleBobble FB)`, which is what this check seems to suggest we do. I don't see how we can determine whether the using declaration should or should not be moved, so I can't really tell what the diagnostic text should be.


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

https://reviews.llvm.org/D55411