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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+  if (loc.isInvalid()) {
----------------
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);
```


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager &manager, SourceLocation loc) {
+  if (loc.isInvalid()) return false;
----------------
nit: LLVM code guideline uses `Camel` for variable names.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:26
+  llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+                 "synchronization|types|utiliy)");
+  return RE.match(Filename);
----------------
typo: utiliy => utility.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:51
+                      InternalDependency->getBeginLoc())) {
+
+    diag(InternalDependency->getBeginLoc(),
----------------
nit: remove the empty line.


================
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:8
+is in a namespace that includes the word “internal”, code is not allowed to 
+depend upon it beaucse it’s an implementation detail. They cannot friend it, 
+include it, you mention it or refer to it in any way. Doing so violates 
----------------
This sentence doesn't parse.


================
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:17
+
+  absl::strings_internal::foo();
+  class foo{
----------------
nit: Please add a trailing comment on the line where it'd trigger the warning.


================
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:18
+  absl::strings_internal::foo();
+  class foo{
+    friend struct absl::container_internal::faa;
----------------
nit:  space between `foo` and `{`.


https://reviews.llvm.org/D50542