[clang] Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity (PR #118511)

2024-12-03 Thread Loïc Joly via cfe-commits 

https://github.com/loic-joly-sonarsource created 
https://github.com/llvm/llvm-project/pull/118511

Before the change, getMemoizationData is used as a key to stop visiting the 
parents, but if a node has no identity, this is nullptr, which means that the 
visit will stop as soon as a second node with no identity is visited.

From ed43bf26057073ba3f1b185838ba95048002980f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Joly?= 
Date: Tue, 3 Dec 2024 17:20:18 +0100
Subject: [PATCH] Fix a bug with the hasAncestor AST matcher when a node has
 several parents without pointer identity.

Before the change, getMemoizationData is used as a key to stop visiting the 
parents, but if a node has no identity, this is nullptr, which means that the 
visit will stop as soon as a second node with no identity is visited.
---
 clang/lib/ASTMatchers/ASTMatchFinder.cpp   |  3 ++-
 .../ASTMatchers/ASTMatchersTraversalTest.cpp   | 18 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 3d01a70395a9bb..a7ef5ae6949537 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1237,7 +1237,8 @@ class MatchASTVisitor : public 
RecursiveASTVisitor,
   // Make sure we do not visit the same node twice.
   // Otherwise, we'll visit the common ancestors as often as there
   // are splits on the way down.
-  if (Visited.insert(Parent.getMemoizationData()).second)
+  if (Parent.getMemoizationData() == nullptr ||
+Visited.insert(Parent.getMemoizationData()).second)
 Queue.push_back(Parent);
 }
 Queue.pop_front();
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp 
b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 1d18869a6b8afc..17b058e4783018 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -5536,6 +5536,24 @@ TEST(HasAncestor, MatchesInImplicitCode) {
 hasAncestor(recordDecl(hasName("A");
 }
 
+TEST(HasAncestor, MatchesWithMultipleParentsWithoutPointerIdentity) {
+EXPECT_TRUE(matches(
+R"cpp(
+template  class Fact {};
+template  class W {};
+template  struct A
+{
+static void f() {
+W> fact12;
+}
+};
+void f() {
+A::f();
+A::f();
+})cpp",
+integerLiteral(hasAncestor(functionDecl();
+}
+
 TEST(HasParent, MatchesOnlyParent) {
   EXPECT_TRUE(matches(
 "void f() { if (true) { int x = 42; } }",

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity (PR #118511)

2024-12-03 Thread Loïc Joly via cfe-commits 

https://github.com/loic-joly-sonarsource updated 
https://github.com/llvm/llvm-project/pull/118511

From 8c6882a360d0f810346dd89f20d8af0ddf0bdfb8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Joly?= 
Date: Tue, 3 Dec 2024 17:20:18 +0100
Subject: [PATCH] Fix a bug with the hasAncestor AST matcher when a node has
 several parents without pointer identity.

Before the change, getMemoizationData is used as a key to stop visiting the 
parents, but if a node has no identity, this is nullptr, which means that the 
visit will stop as soon as a second node with no identity is visited.
---
 clang/lib/ASTMatchers/ASTMatchFinder.cpp   |  3 ++-
 .../ASTMatchers/ASTMatchersTraversalTest.cpp   | 18 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 3d01a70395a9bb..78fbbddb6bb9b1 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1237,7 +1237,8 @@ class MatchASTVisitor : public 
RecursiveASTVisitor,
   // Make sure we do not visit the same node twice.
   // Otherwise, we'll visit the common ancestors as often as there
   // are splits on the way down.
-  if (Visited.insert(Parent.getMemoizationData()).second)
+  if (Parent.getMemoizationData() == nullptr ||
+  Visited.insert(Parent.getMemoizationData()).second)
 Queue.push_back(Parent);
 }
 Queue.pop_front();
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp 
b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 1d18869a6b8afc..fdef08674d091b 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -5536,6 +5536,24 @@ TEST(HasAncestor, MatchesInImplicitCode) {
 hasAncestor(recordDecl(hasName("A");
 }
 
+TEST(HasAncestor, MatchesWithMultipleParentsWithoutPointerIdentity) {
+  EXPECT_TRUE(matches(
+  R"cpp(
+template  class Fact {};
+template  class W {};
+template  struct A
+{
+static void f() {
+W> fact12;
+}
+};
+void f() {
+A::f();
+A::f();
+})cpp",
+  integerLiteral(hasAncestor(functionDecl();
+}
+
 TEST(HasParent, MatchesOnlyParent) {
   EXPECT_TRUE(matches(
 "void f() { if (true) { int x = 42; } }",

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity (PR #118511)

2024-12-03 Thread Loïc Joly via cfe-commits 


@@ -1237,7 +1237,8 @@ class MatchASTVisitor : public 
RecursiveASTVisitor,
   // Make sure we do not visit the same node twice.
   // Otherwise, we'll visit the common ancestors as often as there
   // are splits on the way down.
-  if (Visited.insert(Parent.getMemoizationData()).second)
+  if (Parent.getMemoizationData() == nullptr ||
+  Visited.insert(Parent.getMemoizationData()).second)

loic-joly-sonarsource wrote:

Since this is simply an if and taking an address, I assumed it would be 
optimized. But I applied the suggested change.

https://github.com/llvm/llvm-project/pull/118511
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity (PR #118511)

2024-12-09 Thread Loïc Joly via cfe-commits 

loic-joly-sonarsource wrote:

Hi there. I do not have commit access, but this PR had one approval. Could 
someone with commit access merge it?

https://github.com/llvm/llvm-project/pull/118511
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix a bug with the hasAncestor AST matcher when a node has several parents without pointer identity (PR #118511)

2024-12-03 Thread Loïc Joly via cfe-commits 

https://github.com/loic-joly-sonarsource updated 
https://github.com/llvm/llvm-project/pull/118511

From 8c6882a360d0f810346dd89f20d8af0ddf0bdfb8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Joly?= 
Date: Tue, 3 Dec 2024 17:20:18 +0100
Subject: [PATCH 1/2] Fix a bug with the hasAncestor AST matcher when a node
 has several parents without pointer identity.

Before the change, getMemoizationData is used as a key to stop visiting the 
parents, but if a node has no identity, this is nullptr, which means that the 
visit will stop as soon as a second node with no identity is visited.
---
 clang/lib/ASTMatchers/ASTMatchFinder.cpp   |  3 ++-
 .../ASTMatchers/ASTMatchersTraversalTest.cpp   | 18 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 3d01a70395a9bb..78fbbddb6bb9b1 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1237,7 +1237,8 @@ class MatchASTVisitor : public 
RecursiveASTVisitor,
   // Make sure we do not visit the same node twice.
   // Otherwise, we'll visit the common ancestors as often as there
   // are splits on the way down.
-  if (Visited.insert(Parent.getMemoizationData()).second)
+  if (Parent.getMemoizationData() == nullptr ||
+  Visited.insert(Parent.getMemoizationData()).second)
 Queue.push_back(Parent);
 }
 Queue.pop_front();
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp 
b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 1d18869a6b8afc..fdef08674d091b 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -5536,6 +5536,24 @@ TEST(HasAncestor, MatchesInImplicitCode) {
 hasAncestor(recordDecl(hasName("A");
 }
 
+TEST(HasAncestor, MatchesWithMultipleParentsWithoutPointerIdentity) {
+  EXPECT_TRUE(matches(
+  R"cpp(
+template  class Fact {};
+template  class W {};
+template  struct A
+{
+static void f() {
+W> fact12;
+}
+};
+void f() {
+A::f();
+A::f();
+})cpp",
+  integerLiteral(hasAncestor(functionDecl();
+}
+
 TEST(HasParent, MatchesOnlyParent) {
   EXPECT_TRUE(matches(
 "void f() { if (true) { int x = 42; } }",

From b2a07da335d965b0ce9faa9125362663411f74a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Joly?= 
Date: Tue, 3 Dec 2024 20:16:27 +0100
Subject: [PATCH 2/2] Compute getMemoizationData only once

---
 clang/lib/ASTMatchers/ASTMatchFinder.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 78fbbddb6bb9b1..121e148a429d8d 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1237,8 +1237,8 @@ class MatchASTVisitor : public 
RecursiveASTVisitor,
   // Make sure we do not visit the same node twice.
   // Otherwise, we'll visit the common ancestors as often as there
   // are splits on the way down.
-  if (Parent.getMemoizationData() == nullptr ||
-  Visited.insert(Parent.getMemoizationData()).second)
+  auto Key = Parent.getMemoizationData();
+  if (Key == nullptr || Visited.insert(Key).second)
 Queue.push_back(Parent);
 }
 Queue.pop_front();

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits