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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
