hugoeg added inline comments.

================
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+                     this);
----------------
deannagarcia wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > I think this needs a `not(isExpansionInSystemHeader())` in there, as 
> > > > this is going to trigger on code that includes a header file using an 
> > > > `absl` namespace. If I'm incorrect and users typically include abseil 
> > > > as something other than system includes, you'll have to find a 
> > > > different way to solve this.
> > > Yeah, we have discussed this issue internally.  abseil-user code usually 
> > > includes abseil header like `#include 
> > > "whatever/abseil/base/optimization.h"`, so `IsExpansionInSystemHeader` 
> > > doesn't work for most cases. 
> > > 
> > > We need a way to filter out all headers being a part of abseil library. I 
> > > think we can create a matcher `InExpansionInAbseilHeader` -- basically 
> > > using `IsExpansionInFileMatching` with a regex expression that matches 
> > > typical abseil include path. What do you think?
> > > 
> > > And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) that 
> > > use `InExpansionInAbseilHeader`, we should put it to a common header.
> > I think that is a sensible approach.
> We will begin working on this, I think it will first be implemented in 
> abseil-no-internal-deps but once it looks good I will add it to this patch.
I'm going to go ahead a implement this in ASTMatchers.h and include it on the 
patch for **abseil-no-internal-dep**s


https://reviews.llvm.org/D50580



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to