[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Hugo Gonzalez via Phabricator via cfe-commits]

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