[ASTMatchers] Allow hasName() to look through inline namespaces

[sbenza]

Summary

Allow hasName() to look through inline namespaces.
This will fix the interaction between some clang-tidy checks and libc++.

libc++ defines names in an inline namespace named std::<version_#>.
When we try to match a name using hasName("std::xxx") it fails to match and the clang-tidy check does not work.

Should fix http://llvm.org/PR25804, http://llvm.org/PR25812, http://llvm.org/PR26133.

[aaron.ballman]

lib/ASTMatchers/ASTMatchersInternal.cpp

320 ↗

(On Diff #42755)

Cute, but this won't work with MSVC 2013. You'll get: error C3312: no callable 'begin' function found for type 'initializer-list'

334 ↗

(On Diff #42755)

Should elide braces and format using clang-format.

[yaron.keren]

MSVC 2013 Update 5 accepts for (bool SkipUnwritten : {false, true}).
Possibly changed in one of the Updates?

[sbenza]

Minor fixes

[sbenza]

I am thinking on doing this some other way.
Copying the PrintingPolicy object is very expensive, as it contains quite a few strings and vectors of strings.
In the past I changed this matcher to make no allocations in the common path (ie using SmallString<>, etc) to reduce its cost.
Adding that copy will make it expensive again.

Also, this change is not completely correct. It either uses all namespaces or none of the inline.
It won't partially match.

Eg:

namespace A {
inline namespace B {
inline namespace C {
void F();
}
}
}

We should be able to match F as A::C::F and that doesn't work right now.

lib/ASTMatchers/ASTMatchersInternal.cpp

320 ↗

(On Diff #42755)

Thanks. I assumed it worked because we already allow range-for in LLVM.
Extracted the array into a variable.

334 ↗

(On Diff #42755)

Which braces do you want to remove?
I need these to make the else bind with the outer if().

Applied clang-format.

[aaron.ballman]

lib/ASTMatchers/ASTMatchersInternal.cpp

320–331 ↗

(On Diff #42767)

Extracted the array into a variable.

Good solution to be on the safe side. Range-for isn't the issue with MSVC 2013, it's initializer lists (which really didn't get enough love until 2015).

335 ↗

(On Diff #42767)

Which braces do you want to remove?
I need these to make the else bind with the outer if().

Good point; I think they're fine as-is then.

[yaron.keren]

lib/ASTMatchers/ASTMatchersInternal.cpp

322 ↗

(On Diff #42767)

StringRef not needed, simply

llvm::SmallString<128> NodeName = "::";

323–331 ↗

(On Diff #42767)

You may be able to do without the if

PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
Policy.SuppressUnwrittenScope = SkipUnwritten;
Node.printQualifiedName(OS, Policy);

[sbenza]

Add a faster version of the qualified name matcher.
It falls back to the previous version if it can't handle the name.

lib/ASTMatchers/ASTMatchersInternal.cpp

322 ↗

(On Diff #42767)

I tried it before. It doesn't work.

/usr/local/google/llvm_git/llvm/tools/clang/lib/ASTMatchers/ASTMatchersInternal.cpp:321:28: error: no viable conversion from 'const char [3]' to

  'llvm::SmallString<128>'
llvm::SmallString<128> NodeName = "::";

[Eugene.Zelenko]

It'll be great to finalize fix before 3.8 branching (planned at January 13).

[alexfh]

Manuel, can you take a look at this?

[klimek]

Adding Benjamin for an additional pair of eyes.
Generally looks good.

lib/ASTMatchers/ASTMatchersInternal.cpp

351 ↗

(On Diff #42884)

Perhaps comment that Pattern will be changed to the prefix, so it's clear afterwards.
(I wonder whether returning a tuple might be slightly easier to understand here)

[sbenza]

Rename functions.

lib/ASTMatchers/ASTMatchersInternal.cpp

351 ↗

(On Diff #42884)

Renamed from Match to Consume. Is that better?

[Prazek]

What is the status of it?

[bkramer]

lg

cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h

private:
   /// \brief Unqualified match routine.
   ///
   /// It is much faster than the full match, but it only works for unqualified
   /// matches.
   bool matchesNodeUnqualified(const NamedDecl &Node) const;
 
   /// \brief Full match routine
   ///
+  /// Fast implementation for the simple case of a named declaration at
+  /// namespace or RecordDecl scope.
+  /// It is slower than matchesNodeUnqualified, but faster than
+  /// matchesNodeFullSlow.
+  bool matchesNodeFullFast(const NamedDecl &Node) const;
+
+  /// \brief Full match routine
+  ///
   /// It generates the fully qualified name of the declaration (which is
   /// expensive) before trying to match.
   /// It is slower but simple and works on all cases.
-  bool matchesNodeFull(const NamedDecl &Node) const;
+  bool matchesNodeFullSlow(const NamedDecl &Node) const;
 
   const bool UseUnqualifiedMatch;
   const std::string Name;
 };
 
 /// \brief Matches declarations for QualType and CallExpr.
 ///
 /// Type argument DeclMatcherT is required by PolymorphicMatcherWithParam1 but

cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp

bool AnyOfVariadicOperator(const ast_type_traits::DynTypedNode &DynNode,
   return false;
 }
 
 HasNameMatcher::HasNameMatcher(StringRef NameRef)
     : UseUnqualifiedMatch(NameRef.find("::") == NameRef.npos), Name(NameRef) {
   assert(!Name.empty());
 }
 
-bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl &Node) const {
-  assert(UseUnqualifiedMatch);
-  if (Node.getIdentifier()) {
-    // Simple name.
-    return Name == Node.getName();
-  }
+namespace {
+
+bool ConsumeNameSuffix(StringRef &FullName, StringRef Suffix) {
+  StringRef Name = FullName;
+  if (!Name.endswith(Suffix))
+    return false;
+  Name = Name.drop_back(Suffix.size());
+  if (!Name.empty()) {
+    if (!Name.endswith("::"))
+      return false;
+    Name = Name.drop_back(2);
+  }
+  FullName = Name;
+  return true;
+
+bool ConsumeNodeName(StringRef &Name, const NamedDecl &Node) {
+  // Simple name.
+  if (Node.getIdentifier())
+    return ConsumeNameSuffix(Name, Node.getName());
+
   if (Node.getDeclName()) {
     // Name needs to be constructed.
     llvm::SmallString<128> NodeName;
     llvm::raw_svector_ostream OS(NodeName);
     Node.printName(OS);
-    return Name == OS.str();
+    return ConsumeNameSuffix(Name, OS.str());
+  }
+
+  return ConsumeNameSuffix(Name, "(anonymous)");
+}
+
+}  // namespace
+
+bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl &Node) const {
+  assert(UseUnqualifiedMatch);
+  StringRef NodeName = Name;
+  return ConsumeNodeName(NodeName, Node) && NodeName.empty();
+}
+
+bool HasNameMatcher::matchesNodeFullFast(const NamedDecl &Node) const {
+  // This function is copied and adapted from NamedDecl::printQualifiedName()
+  // By matching each part individually we optimize in a couple of ways:
+  //  - We can exit early on the first failure.
+  //  - We can skip inline/anonymous namespaces without another pass.
+  //  - We print one name at a time, reducing the chance of overflowing the
+  //    inlined space of the SmallString.
+  StringRef Pattern = Name;
+  const bool IsFullyQualified = Pattern.startswith("::");
+
+  // First, match the name.
+  if (!ConsumeNodeName(Pattern, Node))
+    return false;
+
+  // Try to match each declaration context.
+  // We are allowed to skip anonymous and inline namespaces if they don't match.
+  const DeclContext *Ctx = Node.getDeclContext();
+
+  if (Ctx->isFunctionOrMethod())
+    return Pattern.empty() && !IsFullyQualified;
+
+  for (; !Pattern.empty() && Ctx && isa<NamedDecl>(Ctx);
+       Ctx = Ctx->getParent()) {
+    if (const auto *ND = dyn_cast<NamespaceDecl>(Ctx)) {
+      StringRef NSName =
+          ND->isAnonymousNamespace() ? "(anonymous namespace)" : ND->getName();
+
+      // If it matches, continue.
+      if (ConsumeNameSuffix(Pattern, NSName))
+        continue;
+      // If it didn't match but we can skip it, continue.
+      if (ND->isAnonymousNamespace() || ND->isInline())
+        continue;
+
+      return false;
+    }
+    if (const auto *RD = dyn_cast<RecordDecl>(Ctx)) {
+      if (!isa<ClassTemplateSpecializationDecl>(Ctx)) {
+        if (RD->getIdentifier()) {
+          if (ConsumeNameSuffix(Pattern, RD->getName()))
+            continue;
+        } else {
+          llvm::SmallString<128> NodeName;
+          NodeName += StringRef("(anonymous ");
+          NodeName += RD->getKindName();
+          NodeName += ')';
+          if (ConsumeNameSuffix(Pattern, NodeName))
+            continue;
+        }
+
+        return false;
+      }
     }
+
+    // We don't know how to deal with this DeclContext.
+    // Fallback to the slow version of the code.
+    return matchesNodeFullSlow(Node);
+  }
+
+  // If we are fully qualified, we must not have any leftover context.
+  if (IsFullyQualified && Ctx && isa<NamedDecl>(Ctx))
     return false;
+
+  return Pattern.empty();
 }
 
-bool HasNameMatcher::matchesNodeFull(const NamedDecl &Node) const {
+bool HasNameMatcher::matchesNodeFullSlow(const NamedDecl &Node) const {
+  const StringRef Pattern = Name;
+
+  const bool SkipUnwrittenCases[] = {false, true};
+  for (bool SkipUnwritten : SkipUnwrittenCases) {
     llvm::SmallString<128> NodeName = StringRef("::");
     llvm::raw_svector_ostream OS(NodeName);
+
+    if (SkipUnwritten) {
+      PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
+      Policy.SuppressUnwrittenScope = true;
+      Node.printQualifiedName(OS, Policy);
+    } else {
       Node.printQualifiedName(OS);
+    }
+
     const StringRef FullName = OS.str();
-  const StringRef Pattern = Name;
 
-  if (Pattern.startswith("::"))
-    return FullName == Pattern;
+    if (Pattern.startswith("::")) {
+      if (FullName == Pattern)
+        return true;
+    } else if (FullName.endswith(Pattern) &&
+               FullName.drop_back(Pattern.size()).endswith("::")) {
+      return true;
+    }
+  }
 
-  return FullName.endswith(Pattern) &&
-         FullName.drop_back(Pattern.size()).endswith("::");
+  return false;
 }
 
 bool HasNameMatcher::matchesNode(const NamedDecl &Node) const {
-  // FIXME: There is still room for improvement, but it would require copying a
-  // lot of the logic from NamedDecl::printQualifiedName(). The benchmarks do
-  // not show like that extra complexity is needed right now.
+  assert(matchesNodeFullFast(Node) == matchesNodeFullSlow(Node));
   if (UseUnqualifiedMatch) {
-    assert(matchesNodeUnqualified(Node) == matchesNodeFull(Node));
+    assert(matchesNodeUnqualified(Node) == matchesNodeFullFast(Node));
     return matchesNodeUnqualified(Node);
   }
-  return matchesNodeFull(Node);
+  return matchesNodeFullFast(Node);
 }
 
 } // end namespace internal
 } // end namespace ast_matchers
 } // end namespace clang

cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

EXPECT_TRUE(
                  recordDecl(hasName("::B::C"))));
   EXPECT_TRUE(notMatches("class A { class B { class C; }; };",
               recordDecl(hasName("z::A::B::C"))));
   EXPECT_TRUE(
       notMatches("class A { class B { class C; }; };",
                  recordDecl(hasName("A+B::C"))));
 }
 
+TEST(Matcher, HasNameSupportsInlinedNamespaces) {
+  std::string code = "namespace a { inline namespace b { class C; } }";
+  EXPECT_TRUE(matches(code, recordDecl(hasName("a::b::C"))));
+  EXPECT_TRUE(matches(code, recordDecl(hasName("a::C"))));
+  EXPECT_TRUE(matches(code, recordDecl(hasName("::a::b::C"))));
+  EXPECT_TRUE(matches(code, recordDecl(hasName("::a::C"))));
+}
+
+TEST(Matcher, HasNameSupportsAnonymousNamespaces) {
+  std::string code = "namespace a { namespace { class C; } }";
+  EXPECT_TRUE(
+      matches(code, recordDecl(hasName("a::(anonymous namespace)::C"))));
+  EXPECT_TRUE(matches(code, recordDecl(hasName("a::C"))));
+  EXPECT_TRUE(
+      matches(code, recordDecl(hasName("::a::(anonymous namespace)::C"))));
+  EXPECT_TRUE(matches(code, recordDecl(hasName("::a::C"))));
+}
+
+TEST(Matcher, HasNameSupportsAnonymousOuterClasses) {
+  EXPECT_TRUE(matches("class A { class { class C; } x; };",
+                      recordDecl(hasName("A::(anonymous class)::C"))));
+  EXPECT_TRUE(matches("class A { class { class C; } x; };",
+                      recordDecl(hasName("::A::(anonymous class)::C"))));
+  EXPECT_FALSE(matches("class A { class { class C; } x; };",
+                       recordDecl(hasName("::A::C"))));
+  EXPECT_TRUE(matches("class A { struct { class C; } x; };",
+                      recordDecl(hasName("A::(anonymous struct)::C"))));
+  EXPECT_TRUE(matches("class A { struct { class C; } x; };",
+                      recordDecl(hasName("::A::(anonymous struct)::C"))));
+  EXPECT_FALSE(matches("class A { struct { class C; } x; };",
+                       recordDecl(hasName("::A::C"))));
+}
+
+TEST(Matcher, HasNameSupportsFunctionScope) {
+  std::string code =
+      "namespace a { void F(int a) { struct S { int m; }; int i; } }";
+  EXPECT_TRUE(matches(code, varDecl(hasName("i"))));
+  EXPECT_FALSE(matches(code, varDecl(hasName("F()::i"))));
+
+  EXPECT_TRUE(matches(code, fieldDecl(hasName("m"))));
+  EXPECT_TRUE(matches(code, fieldDecl(hasName("S::m"))));
+  EXPECT_TRUE(matches(code, fieldDecl(hasName("F(int)::S::m"))));
+  EXPECT_TRUE(matches(code, fieldDecl(hasName("a::F(int)::S::m"))));
+  EXPECT_TRUE(matches(code, fieldDecl(hasName("::a::F(int)::S::m"))));
+}
+
 TEST(Matcher, IsDefinition) {
   DeclarationMatcher DefinitionOfClassA =
       recordDecl(hasName("A"), isDefinition());
   EXPECT_TRUE(matches("class A {};", DefinitionOfClassA));
   EXPECT_TRUE(notMatches("class A;", DefinitionOfClassA));
 
   DeclarationMatcher DefinitionOfVariableA =
       varDecl(hasName("a"), isDefinition());