[clang] [clang][ASTMatchers] Fix forEachArgumentWithParam* for deducing "this" operator calls (PR #84887)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 12 01:30:47 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Balazs Benics (steakhal)
<details>
<summary>Changes</summary>
This is a follow-up commit of #<!-- -->84446.
In this patch, I demonstrate that `forEachArgumentWithParam` and `forEachArgumentWithParamType` did not correctly handle the presence of the explicit object parameter for operator calls.
Prior to this patch, the matcher would skip the first (and only) argument of the operator call if the explicit object param was used.
Note that I had to move the definition of `isExplicitObjectMemberFunction`, to be declared before the matcher I fix to be visible.
I also had to do some gymnastics for passing the language standard version command-line flags to the invocation as `matchAndVerifyResultTrue` wasn't really considered for non-c++11 code. See the that it always prepends `-std=gnu++11` to the command-line arguments. I workarounded it by accepting extra args, which get appended, thus possibly overriding the hardcoded arguments.
I'm not sure if this qualifies for backporting to clang-18, but I figure it might be useful for some vendors (like us).
But we are also happy to cherry-pick this fix to downstream. Let me know if you want this to be backported or not.
---
Full diff: https://github.com/llvm/llvm-project/pull/84887.diff
4 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+2)
- (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+31-28)
- (modified) clang/unittests/ASTMatchers/ASTMatchersTest.h (+23-13)
- (modified) clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp (+63)
``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 88e552d5c46113..dcc0c2f2cc40c6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -447,6 +447,8 @@ AST Matchers
- ``isInStdNamespace`` now supports Decl declared with ``extern "C++"``.
- Add ``isExplicitObjectMemberFunction``.
+- Fixed ``forEachArgumentWithParam`` and ``forEachArgumentWithParamType`` to
+ not skip the explicit object parameter for operator calls.
clang-format
------------
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 96dbcdc344e131..2f71053d030f68 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -5032,6 +5032,25 @@ AST_POLYMORPHIC_MATCHER_P2(hasParameter,
&& InnerMatcher.matches(*Node.parameters()[N], Finder, Builder));
}
+/// Matches if the given method declaration declares a member function with an
+/// explicit object parameter.
+///
+/// Given
+/// \code
+/// struct A {
+/// int operator-(this A, int);
+/// void fun(this A &&self);
+/// static int operator()(int);
+/// int operator+(int);
+/// };
+/// \endcode
+///
+/// cxxMethodDecl(isExplicitObjectMemberFunction()) matches the first two
+/// methods but not the last two.
+AST_MATCHER(CXXMethodDecl, isExplicitObjectMemberFunction) {
+ return Node.isExplicitObjectMemberFunction();
+}
+
/// Matches all arguments and their respective ParmVarDecl.
///
/// Given
@@ -5060,10 +5079,12 @@ AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParam,
// argument of the method which should not be matched against a parameter, so
// we skip over it here.
BoundNodesTreeBuilder Matches;
- unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
- .matches(Node, Finder, &Matches)
- ? 1
- : 0;
+ unsigned ArgIndex =
+ cxxOperatorCallExpr(
+ callee(cxxMethodDecl(unless(isExplicitObjectMemberFunction()))))
+ .matches(Node, Finder, &Matches)
+ ? 1
+ : 0;
int ParamIndex = 0;
bool Matched = false;
for (; ArgIndex < Node.getNumArgs(); ++ArgIndex) {
@@ -5121,11 +5142,12 @@ AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParamType,
// argument of the method which should not be matched against a parameter, so
// we skip over it here.
BoundNodesTreeBuilder Matches;
- unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
- .matches(Node, Finder, &Matches)
- ? 1
- : 0;
-
+ unsigned ArgIndex =
+ cxxOperatorCallExpr(
+ callee(cxxMethodDecl(unless(isExplicitObjectMemberFunction()))))
+ .matches(Node, Finder, &Matches)
+ ? 1
+ : 0;
const FunctionProtoType *FProto = nullptr;
if (const auto *Call = dyn_cast<CallExpr>(&Node)) {
@@ -6366,25 +6388,6 @@ AST_MATCHER(CXXMethodDecl, isConst) {
return Node.isConst();
}
-/// Matches if the given method declaration declares a member function with an
-/// explicit object parameter.
-///
-/// Given
-/// \code
-/// struct A {
-/// int operator-(this A, int);
-/// void fun(this A &&self);
-/// static int operator()(int);
-/// int operator+(int);
-/// };
-/// \endcode
-///
-/// cxxMethodDecl(isExplicitObjectMemberFunction()) matches the first two
-/// methods but not the last two.
-AST_MATCHER(CXXMethodDecl, isExplicitObjectMemberFunction) {
- return Node.isExplicitObjectMemberFunction();
-}
-
/// Matches if the given method declaration declares a copy assignment
/// operator.
///
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTest.h b/clang/unittests/ASTMatchers/ASTMatchersTest.h
index 1ed1b5958a8b3a..e9812995315741 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTest.h
+++ b/clang/unittests/ASTMatchers/ASTMatchersTest.h
@@ -293,7 +293,8 @@ testing::AssertionResult notMatchesWithOpenMP51(const Twine &Code,
template <typename T>
testing::AssertionResult matchAndVerifyResultConditionally(
const Twine &Code, const T &AMatcher,
- std::unique_ptr<BoundNodesCallback> FindResultVerifier, bool ExpectResult) {
+ std::unique_ptr<BoundNodesCallback> FindResultVerifier, bool ExpectResult,
+ ArrayRef<std::string> Args = {}, StringRef Filename = "input.cc") {
bool VerifiedResult = false;
MatchFinder Finder;
VerifyMatch VerifyVerifiedResult(std::move(FindResultVerifier),
@@ -304,9 +305,13 @@ testing::AssertionResult matchAndVerifyResultConditionally(
// Some tests use typeof, which is a gnu extension. Using an explicit
// unknown-unknown triple is good for a large speedup, because it lets us
// avoid constructing a full system triple.
- std::vector<std::string> Args = {"-std=gnu++11", "-target",
- "i386-unknown-unknown"};
- if (!runToolOnCodeWithArgs(Factory->create(), Code, Args)) {
+ std::vector<std::string> CompileArgs = {"-std=gnu++11", "-target",
+ "i386-unknown-unknown"};
+ // Append additional arguments at the end to allow overriding the default
+ // choices that we made above.
+ llvm::copy(Args, std::back_inserter(CompileArgs));
+
+ if (!runToolOnCodeWithArgs(Factory->create(), Code, CompileArgs, Filename)) {
return testing::AssertionFailure() << "Parsing error in \"" << Code << "\"";
}
if (!VerifiedResult && ExpectResult) {
@@ -319,8 +324,8 @@ testing::AssertionResult matchAndVerifyResultConditionally(
VerifiedResult = false;
SmallString<256> Buffer;
- std::unique_ptr<ASTUnit> AST(
- buildASTFromCodeWithArgs(Code.toStringRef(Buffer), Args));
+ std::unique_ptr<ASTUnit> AST(buildASTFromCodeWithArgs(
+ Code.toStringRef(Buffer), CompileArgs, Filename));
if (!AST.get())
return testing::AssertionFailure()
<< "Parsing error in \"" << Code << "\" while building AST";
@@ -339,19 +344,24 @@ testing::AssertionResult matchAndVerifyResultConditionally(
// FIXME: Find better names for these functions (or document what they
// do more precisely).
template <typename T>
-testing::AssertionResult matchAndVerifyResultTrue(
- const Twine &Code, const T &AMatcher,
- std::unique_ptr<BoundNodesCallback> FindResultVerifier) {
- return matchAndVerifyResultConditionally(Code, AMatcher,
- std::move(FindResultVerifier), true);
+testing::AssertionResult
+matchAndVerifyResultTrue(const Twine &Code, const T &AMatcher,
+ std::unique_ptr<BoundNodesCallback> FindResultVerifier,
+ ArrayRef<std::string> Args = {},
+ StringRef Filename = "input.cc") {
+ return matchAndVerifyResultConditionally(
+ Code, AMatcher, std::move(FindResultVerifier),
+ /*ExpectResult=*/true, Args, Filename);
}
template <typename T>
testing::AssertionResult matchAndVerifyResultFalse(
const Twine &Code, const T &AMatcher,
- std::unique_ptr<BoundNodesCallback> FindResultVerifier) {
+ std::unique_ptr<BoundNodesCallback> FindResultVerifier,
+ ArrayRef<std::string> Args = {}, StringRef Filename = "input.cc") {
return matchAndVerifyResultConditionally(
- Code, AMatcher, std::move(FindResultVerifier), false);
+ Code, AMatcher, std::move(FindResultVerifier),
+ /*ExpectResult=*/false, Args, Filename);
}
// Implements a run method that returns whether BoundNodes contains a
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 6911d7600a7188..f198dc71eb8337 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -985,6 +985,38 @@ TEST(ForEachArgumentWithParam, HandlesBoundNodesForNonMatches) {
std::make_unique<VerifyIdIsBoundTo<VarDecl>>("v", 4)));
}
+TEST_P(ASTMatchersTest,
+ ForEachArgumentWithParamMatchesExplicitObjectParamOnOperatorCalls) {
+ if (!GetParam().isCXX23OrLater()) {
+ return;
+ }
+
+ auto DeclRef = declRefExpr(to(varDecl().bind("declOfArg"))).bind("arg");
+ auto SelfParam = parmVarDecl().bind("param");
+ StatementMatcher CallExpr =
+ callExpr(forEachArgumentWithParam(DeclRef, SelfParam));
+
+ StringRef S = R"cpp(
+ struct A {
+ int operator()(this const A &self);
+ };
+ A obj;
+ int global = obj();
+ )cpp";
+
+ auto Args = GetParam().getCommandLineArgs();
+ auto Filename = getFilenameForTesting(GetParam().Language);
+
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ S, CallExpr,
+ std::make_unique<VerifyIdIsBoundTo<ParmVarDecl>>("param", "self"), Args,
+ Filename));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ S, CallExpr,
+ std::make_unique<VerifyIdIsBoundTo<VarDecl>>("declOfArg", "obj"), Args,
+ Filename));
+}
+
TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) {
StatementMatcher ArgumentY =
declRefExpr(to(varDecl(hasName("y")))).bind("arg");
@@ -1168,6 +1200,37 @@ TEST(ForEachArgumentWithParamType, MatchesVariadicFunctionPtrCalls) {
S, CallExpr, std::make_unique<VerifyIdIsBoundTo<DeclRefExpr>>("arg")));
}
+TEST_P(ASTMatchersTest,
+ ForEachArgumentWithParamTypeMatchesExplicitObjectParamOnOperatorCalls) {
+ if (!GetParam().isCXX23OrLater()) {
+ return;
+ }
+
+ auto DeclRef = declRefExpr(to(varDecl().bind("declOfArg"))).bind("arg");
+ auto SelfTy = qualType(asString("const A &")).bind("selfType");
+ StatementMatcher CallExpr =
+ callExpr(forEachArgumentWithParamType(DeclRef, SelfTy));
+
+ StringRef S = R"cpp(
+ struct A {
+ int operator()(this const A &self);
+ };
+ A obj;
+ int global = obj();
+ )cpp";
+
+ auto Args = GetParam().getCommandLineArgs();
+ auto Filename = getFilenameForTesting(GetParam().Language);
+
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ S, CallExpr, std::make_unique<VerifyIdIsBoundTo<QualType>>("selfType"),
+ Args, Filename));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ S, CallExpr,
+ std::make_unique<VerifyIdIsBoundTo<VarDecl>>("declOfArg", "obj"), Args,
+ Filename));
+}
+
TEST(QualType, hasCanonicalType) {
EXPECT_TRUE(notMatches("typedef int &int_ref;"
"int a;"
``````````
</details>
https://github.com/llvm/llvm-project/pull/84887
More information about the cfe-commits
mailing list