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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
