hokein added inline comments.
================ Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23 + + Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"), + this); ---------------- JonasToth wrote: > hugoeg wrote: > > 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 > In principle it is ok, but I don't think ASTMatchers.h is the correct place, > because it is only of use to clang-tidy. > > There is a `util` directory in clang-tidy for this kind of stuff. Defining > you own matchers works the same as in ASTMatchers, you can grep through > clang-tidy checks (e.g. `AST_MATCHER`) to have some examples of private > matchers. `ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. We have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific matchers. I'm not sure whether it is reasonable to put abseil-specific matchers there. Maybe we create a `AbseilMatcher.h` file in `clang-tidy/abseil` directory? https://reviews.llvm.org/D50580 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits