[clang] Ignore inline namespace for `hasName` #109147

alejandro-alvarez-sonarsource · 2024-09-18T13:59:47Z

Add a new flag AlwaysSuppressInlineNamespace to PrintingPolicy that is explicit
about always removing inline namespaces regardless of ambiguity.

Specializing a template from an inline namespace should be transparent. For instance

namespace foo {
    inline namespace v1 {
        template<typename A>
        void function(A&);
    }
}

namespace foo {
    template<>
    void function<int>(int&);
}

hasName should match both declarations of foo::function.

Makes the behavior of matchesNodeFullSlow and matchesNodeFullFast consistent, fixing
an assert inside HasNameMatcher::matchesNode.

Add a new flag AlwaysSuppressInlineNamespace to PrintingPolicy that is explicit about *always* removing inline namespaces regardless of ambiguity. Specializing a template from an inline namespace should be transparent to said namespace. For instance ``` namespace foo { inline namespace v1 { template<typename A> void function(A&); } } namespace foo { template<> void function<int>(int&); } ``` `hasName` should match both declarations of `foo::function`. Makes the behavior of `matchesNodeFullSlow` and `matchesNodeFullFast` consistent.

llvmbot · 2024-09-18T14:00:23Z

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

Changes

Add a new flag AlwaysSuppressInlineNamespace to PrintingPolicy that is explicit
about always removing inline namespaces regardless of ambiguity.

Specializing a template from an inline namespace should be transparent. For instance

namespace foo {
    inline namespace v1 {
        template&lt;typename A&gt;
        void function(A&amp;);
    }
}

namespace foo {
    template&lt;&gt;
    void function&lt;int&gt;(int&amp;);
}

hasName should match both declarations of foo::function.

Makes the behavior of matchesNodeFullSlow and matchesNodeFullFast consistent, fixing
an assert inside HasNameMatcher::matchesNode.

Full diff: https://github.com/llvm/llvm-project/pull/109147.diff

4 Files Affected:

(modified) clang/include/clang/AST/PrettyPrinter.h (+8-3)
(modified) clang/lib/AST/Decl.cpp (+5-2)
(modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+1)
(modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+29)

diff --git a/clang/include/clang/AST/PrettyPrinter.h b/clang/include/clang/AST/PrettyPrinter.h
index 332ac3c6a004a9..a671211bf3ef93 100644
--- a/clang/include/clang/AST/PrettyPrinter.h
+++ b/clang/include/clang/AST/PrettyPrinter.h
@@ -61,9 +61,9 @@ struct PrintingPolicy {
         SuppressTagKeyword(LO.CPlusPlus), IncludeTagDefinition(false),
         SuppressScope(false), SuppressUnwrittenScope(false),
         SuppressInlineNamespace(true), SuppressElaboration(false),
-        SuppressInitializers(false), ConstantArraySizeAsWritten(false),
-        AnonymousTagLocations(true), SuppressStrongLifetime(false),
-        SuppressLifetimeQualifiers(false),
+        AlwaysSuppressInlineNamespace(false), SuppressInitializers(false),
+        ConstantArraySizeAsWritten(false), AnonymousTagLocations(true),
+        SuppressStrongLifetime(false), SuppressLifetimeQualifiers(false),
         SuppressTemplateArgsInCXXConstructors(false),
         SuppressDefaultTemplateArgs(true), Bool(LO.Bool),
         Nullptr(LO.CPlusPlus11 || LO.C23), NullptrTypeInNamespace(LO.CPlusPlus),
@@ -151,6 +151,11 @@ struct PrintingPolicy {
   LLVM_PREFERRED_TYPE(bool)
   unsigned SuppressElaboration : 1;
 
+  /// Suppress printing parts of scope specifiers that correspond
+  /// to inline namespaces, even if the name is ambiguous with the specifier
+  /// removed.
+  unsigned AlwaysSuppressInlineNamespace : 1;
+
   /// Suppress printing of variable initializers.
   ///
   /// This flag is used when printing the loop variable in a for-range
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index a14b1b33d35efc..a3ac12d58ed044 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -1737,8 +1737,11 @@ void NamedDecl::printNestedNameSpecifier(raw_ostream &OS,
       continue;
 
     // Suppress inline namespace if it doesn't make the result ambiguous.
-    if (P.SuppressInlineNamespace && Ctx->isInlineNamespace() && NameInScope &&
-        cast<NamespaceDecl>(Ctx)->isRedundantInlineQualifierFor(NameInScope))
+    if (Ctx->isInlineNamespace() && NameInScope &&
+        (P.AlwaysSuppressInlineNamespace ||
+         (P.SuppressInlineNamespace &&
+          cast<NamespaceDecl>(Ctx)->isRedundantInlineQualifierFor(
+              NameInScope))))
       continue;
 
     // Skip non-named contexts such as linkage specifications and ExportDecls.
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 06309d327896b3..731fc97707eee7 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -656,6 +656,7 @@ bool HasNameMatcher::matchesNodeFullSlow(const NamedDecl &Node) const {
     PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
     Policy.SuppressUnwrittenScope = SkipUnwritten;
     Policy.SuppressInlineNamespace = SkipUnwritten;
+    Policy.AlwaysSuppressInlineNamespace = SkipUnwritten;
     Node.printQualifiedName(OS, Policy);
 
     const StringRef FullName = OS.str();
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index 611e1f9ba5327c..d696375547acce 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2599,6 +2599,35 @@ TEST_P(ASTMatchersTest, HasName_MatchesInlinedNamespaces) {
   EXPECT_TRUE(matches(code, recordDecl(hasName("::a::C"))));
 }
 
+TEST_P(ASTMatchersTest, HasName_MatchesSpecializedInlinedNamespace) {
+  if (!GetParam().isCXX11OrLater()) {
+    return;
+  }
+
+  StringRef code = R"(
+namespace a {
+    inline namespace v1 {
+        template<typename T> T foo(T);
+    }
+}
+
+namespace a {
+    enum Tag{T1, T2};
+
+    template <Tag, typename T> T foo(T);
+}
+
+auto v1 = a::foo(1);
+auto v2 = a::foo<a::T1>(1);
+)";
+  EXPECT_TRUE(matches(
+      code, varDecl(hasName("v1"), hasDescendant(callExpr(callee(
+                                       functionDecl(hasName("::a::foo"))))))));
+  EXPECT_TRUE(matches(
+      code, varDecl(hasName("v2"), hasDescendant(callExpr(callee(
+                                       functionDecl(hasName("::a::foo"))))))));
+}
+
 TEST_P(ASTMatchersTest, HasName_MatchesAnonymousNamespaces) {
   if (!GetParam().isCXX()) {
     return;

erichkeane

I have no problem with the code here, but don't know enough about ASTMatchers to be able to review this for 'well justified'. Do we have someone besides Aaron who does?

Also, it needs a release note I think for the matcher change.

Sirraide · 2024-09-23T20:02:52Z

I can’t help w/ AST matchers either unfortunately.

…Matcher

alejandro-alvarez-sonarsource · 2024-09-24T06:53:05Z

Also, it needs a release note I think for the matcher change.

Added.

I have no problem with the code here, but don't know enough about ASTMatchers to be able to review this for 'well justified'. Do we have someone besides Aaron who does?

FWIW this is an assertion error we saw downstream on debug builds with a snippet like the following (reduced), when trying to find std::move.

namespace std {

inline namespace __1 {
template <class T> T &&move(T &&t) noexcept;

} // namespace __1
} // namespace std

namespace WTF {
enum CheckMoveParameterTag {};
}

namespace std {
template <WTF::CheckMoveParameterTag, typename T>
inline constexpr T &&move(T &&value) {
  return move(value);
}
} // namespace std

template <typename T> void f1(T t) { std::move(t); }

template void f1<int>(int);

The original code comes from WebKit

alejandro-alvarez-sonarsource · 2024-10-09T11:40:29Z

Ping (@PiotrZSL ?)

PiotrZSL

Code looks fine.
Only thing is that existence of SuppressInlineNamespace and AlwaysSuppressInlineNamespace looks redundant.

Probably one should be named SuppressRedundantInlineNamespace and other SuppressAllInlineNamespace. Otherwise it looks strange. Or even SuppressInlineNamespace should be changed from bool to some enum like None, Redundant, All.

Make sure that all current test (specially from clang-tidy) still pass.

AaronBallman · 2024-10-09T18:30:20Z

Or even SuppressInlineNamespace should be changed from bool to some enum like None, Redundant, All.

I think this is the suggestion I'd prefer; it's cleaner than adding a second boolean option.

alejandro-alvarez-sonarsource · 2024-10-10T10:24:52Z

Or even SuppressInlineNamespace should be changed from bool to some enum like None, Redundant, All.

I think this is the suggestion I'd prefer; it's cleaner than adding a second boolean option.

Agreed. And changes.

Make sure that all current test (specially from clang-tidy) still pass.

I have run both check-clang and check-clang-tools (locally) without failures.

github-actions · 2024-10-10T10:27:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

alejandro-alvarez-sonarsource · 2024-10-11T06:43:42Z

I don't think the failure is related to this PR?

27: clang: error: unable to execute command: Executable "llvm-mc" doesn't exist!

P.S It seems to me llvm-mc is missing on the list of dependencies for clang tests.

AaronBallman

LGTM with some minor corrections, thank you!

And yeah, the precommit CI failure is unrelated to your changes.

Do you need someone to commit these changes for you? (If so, I can apply the changes myself and land them.)

AaronBallman · 2024-10-11T12:00:43Z

clang/include/clang/AST/PrettyPrinter.h

  /// removed.
-  LLVM_PREFERRED_TYPE(bool)
-  unsigned SuppressInlineNamespace : 1;
+  SuppressInlineNamespaceMode SuppressInlineNamespace : 2;


Suggested change

SuppressInlineNamespaceMode SuppressInlineNamespace : 2;

LLVM_PREFERRD_TYPE(SuppressInlineNamespaceMode)

unsigned SuppressInlineNamespace : 2;

Applied by hand, since there was a typo (PREFERRD vs PREFERRED)

Oops, good catch! :-D

clang/lib/AST/Decl.cpp

clang/docs/ReleaseNotes.rst

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

alejandro-alvarez-sonarsource · 2024-10-11T12:42:53Z

Do you need someone to commit these changes for you? (If so, I can apply the changes myself and land them.)

Yes, please, I can't merge myself.

…ent-indentonly * llvm-trunk/main: (6379 commits) [gn build] Port 1c94388 [RISCV] Introduce VLOptimizer pass (llvm#108640) [mlir][vector] Add more tests for ConvertVectorToLLVM (7/n) (llvm#111895) [libc++] Add output groups to run-buildbot (llvm#111739) [libc++abi] Remove unused LIBCXXABI_LIBCXX_INCLUDES CMake option (llvm#111824) [clang] Ignore inline namespace for `hasName` (llvm#109147) [AArch64] Disable consecutive store merging when Neon is unavailable (llvm#111519) [lldb] Fix finding make tool for tests (llvm#111980) Turn `-Wdeprecated-literal-operator` on by default (llvm#111027) [AMDGPU] Rewrite RegSeqNames using !foreach. NFC. (llvm#111994) Revert "Reland: [clang] Finish implementation of P0522 (llvm#111711)" Revert "[clang] CWG2398: improve overload resolution backwards compat (llvm#107350)" Revert "[clang] Implement TTP P0522 pack matching for deduced function template calls. (llvm#111457)" [Clang] Replace Intrinsic::getDeclaration with getOrInsertDeclaration (llvm#111990) Revert "[NVPTX] Prefer prmt.b32 over bfi.b32 (llvm#110766)" [RISCV] Add DAG combine to turn (sub (shl X, 8-Y), (shr X, Y)) into orc.b (llvm#111828) [libc] Fix compilation of new trig functions (llvm#111987) [NFC] Rename `Intrinsic::getDeclaration` to `getOrInsertDeclaration` (llvm#111752) [NFC][CodingStandard] Add additional example for if-else brace rule (llvm#111733) CodeGen: Remove redundant REQUIRES registered-target from tests (llvm#111982) ...

Add a new enumeration `SuppressInlineNamespaceMode` to `PrintingPolicy` that is explicit about how to handle inline namespaces. `SuppressInlineNamespace` uses that enumeration now instead of a Boolean value. Specializing a template from an inline namespace should be transparent. For instance ``` namespace foo { inline namespace v1 { template<typename A> void function(A&); } } namespace foo { template<> void function<int>(int&); } ``` `hasName` should match both declarations of `foo::function`. Makes the behavior of `matchesNodeFullSlow` and `matchesNodeFullFast` consistent, fixing an assert inside `HasNameMatcher::matchesNode`.

…lineQualifierFor(). (#115196) We observed 2X slowdown in lldb's expression evaluation with #109147 in some cases. It turns out that calling `isRedundantInlineQualifierFor` is quite expensive. Using short-circuit evaluation in the if statement to avoid unnecessary calls to that function.

…lineQualifierFor(). (llvm#115196) We observed 2X slowdown in lldb's expression evaluation with llvm#109147 in some cases. It turns out that calling `isRedundantInlineQualifierFor` is quite expensive. Using short-circuit evaluation in the if statement to avoid unnecessary calls to that function.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 18, 2024

shafik requested review from erichkeane and Sirraide September 21, 2024 02:43

erichkeane reviewed Sep 23, 2024

View reviewed changes

Sirraide requested a review from AaronBallman September 23, 2024 20:02

Sirraide removed their request for review September 23, 2024 20:03

alejandro-alvarez-sonarsource added 2 commits September 24, 2024 08:45

Merge remote-tracking branch 'upstream/main' into aa/upstream/HasName…

d391541

…Matcher

Add release notes

bd40141

steakhal requested a review from PiotrZSL September 25, 2024 09:05

PiotrZSL reviewed Oct 9, 2024

View reviewed changes

alejandro-alvarez-sonarsource added 3 commits October 10, 2024 08:57

Merge branch 'main' into aa/upstream/HasNameMatcher

17b5893

Use an enum instead of two flags

3bb9d07

Merge branch 'main' into aa/upstream/HasNameMatcher

1898d4a

llvmbot added clang:codegen debuginfo labels Oct 10, 2024

alejandro-alvarez-sonarsource added 3 commits October 10, 2024 13:45

Fix formatting

a196be3

Fix regression and typo

23837c6

Fix formatting

c3ce31c

Merge branch 'main' into aa/upstream/HasNameMatcher

671d8b0

AaronBallman approved these changes Oct 11, 2024

View reviewed changes

alejandro-alvarez-sonarsource and others added 2 commits October 11, 2024 14:29

Apply suggestions from code review

2ac1a6d

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Apply feedback

21f66eb

AaronBallman merged commit bd12729 into llvm:main Oct 11, 2024
7 of 9 checks passed

ZequanWu mentioned this pull request Nov 6, 2024

[clang] Avoid unnecessary call to clang::NamespaceDecl::isRedundantInlineQualifierFor(). #115196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Ignore inline namespace for `hasName` #109147

[clang] Ignore inline namespace for `hasName` #109147

alejandro-alvarez-sonarsource commented Sep 18, 2024

llvmbot commented Sep 18, 2024 •

edited

Loading

erichkeane left a comment

Sirraide commented Sep 23, 2024

alejandro-alvarez-sonarsource commented Sep 24, 2024

alejandro-alvarez-sonarsource commented Oct 9, 2024

PiotrZSL left a comment

AaronBallman commented Oct 9, 2024

alejandro-alvarez-sonarsource commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 •

edited

Loading

alejandro-alvarez-sonarsource commented Oct 11, 2024 •

edited

Loading

AaronBallman left a comment

AaronBallman Oct 11, 2024

alejandro-alvarez-sonarsource Oct 11, 2024

AaronBallman Oct 11, 2024

alejandro-alvarez-sonarsource commented Oct 11, 2024

	SuppressInlineNamespaceMode SuppressInlineNamespace : 2;
	LLVM_PREFERRD_TYPE(SuppressInlineNamespaceMode)
	unsigned SuppressInlineNamespace : 2;

[clang] Ignore inline namespace for hasName #109147

[clang] Ignore inline namespace for hasName #109147

Conversation

alejandro-alvarez-sonarsource commented Sep 18, 2024

llvmbot commented Sep 18, 2024 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

Sirraide commented Sep 23, 2024

alejandro-alvarez-sonarsource commented Sep 24, 2024

alejandro-alvarez-sonarsource commented Oct 9, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

AaronBallman commented Oct 9, 2024

alejandro-alvarez-sonarsource commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 • edited Loading

alejandro-alvarez-sonarsource commented Oct 11, 2024 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Oct 11, 2024

Choose a reason for hiding this comment

alejandro-alvarez-sonarsource Oct 11, 2024

Choose a reason for hiding this comment

AaronBallman Oct 11, 2024

Choose a reason for hiding this comment

alejandro-alvarez-sonarsource commented Oct 11, 2024

[clang] Ignore inline namespace for `hasName` #109147

[clang] Ignore inline namespace for `hasName` #109147

llvmbot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

alejandro-alvarez-sonarsource commented Oct 11, 2024 •

edited

Loading