hugoeg marked an inline comment as done. hugoeg added inline comments.
================ Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20 + +bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){ + if (loc.isInvalid()) { ---------------- hokein wrote: > hugoeg wrote: > > hokein wrote: > > > I think we can make it as an ASTMatcher instead of a function like: > > > > > > ``` > > > AST_POLYMORPHIC_MATCHER_P(isInAbseilFile, > > > AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, > > > TypeLoc)) { > > > // your code here. > > > } > > > ``` > > Unfortunately, I do not think we can. That was the way I originally tried > > to implement it. It worked on no-namespace-check, but not in this one. This > > is because as an AST_POLYMORPHIC_MATCHER_P, we are trying to match an usage > > of a Decl, not the Decl itself and since we are matching a TypeLoc in > > no-internal-deps-check, not really the usage, it doesn't work. > > > > As a result, I modified my original implementation, which you will see in > > no-namespace-check and turned it into IsInAbseilFile(SourceManager&, > > SourceLocation), which is just takes a source location and returns whether > > the location we matched is in an Abseil file > Could you explain a bit more why it won't work? I don't understand why it > doesn't work. > > Basically you define the matcher like: > > ``` > AST_POLYMORPHIC_MATCHER( > isInAbseilFile, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc, > NestedNameSpecifierLoc)) { > auto &SourceManager = Finder->getASTContext().getSourceManager(); > SourceLocation loc = Node.getBeginLoc(); > if (loc.isInvalid()) return false; > const FileEntry *FileEntry = > SourceManager.getFileEntryForID(SourceManager.getFileID(loc)); > if (!FileEntry) return false; > StringRef Filename = FileEntry->getName(); > llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|" > "synchronization|types|utiliy)"); > return RE.match(Filename); > } > ``` > > And use it for example in your check > > ``` > Finder->addMatcher(nestedNameSpecifierLoc( > loc(specifiesNamespace(namespaceDecl( > matchesName("internal"), > hasParent(namespaceDecl(hasName("absl")))))), > unless(isInAbseilFile())) > .bind("InternalDep"), > this); > ``` You're right, this implementation seems to work, I seemed to have a simple logic error in my original implementation, the new diff will include this version https://reviews.llvm.org/D50542 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits