[clang] fe8cf2f - [clang][ASTMatchers] Fix forEachArgumentWithParam* for deducing "this" operator calls (#84887)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 12 09:31:25 PDT 2024
Author: Balazs Benics
Date: 2024-03-12T17:31:22+01:00
New Revision: fe8cf2fc9090d3d56fa1a6bb4b70bd38277874eb
URL: https://github.com/llvm/llvm-project/commit/fe8cf2fc9090d3d56fa1a6bb4b70bd38277874eb
DIFF: https://github.com/llvm/llvm-project/commit/fe8cf2fc9090d3d56fa1a6bb4b70bd38277874eb.diff
LOG: [clang][ASTMatchers] Fix forEachArgumentWithParam* for deducing "this" operator calls (#84887)
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 (probably not
because its not a crash, but a semantic problem), 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.
CPP-5074
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/unittests/ASTMatchers/ASTMatchersTest.h
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6c30af304d981d..2842b63197ff1d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -450,6 +450,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;"
More information about the cfe-commits
mailing list