diff --git a/clang/include/clang/Tooling/Transformer/Stencil.h b/clang/include/clang/Tooling/Transformer/Stencil.h --- a/clang/include/clang/Tooling/Transformer/Stencil.h +++ b/clang/include/clang/Tooling/Transformer/Stencil.h @@ -82,7 +82,6 @@ /// If \p ExprId is of pointer type, constructs an idiomatic dereferencing of /// the expression bound to \p ExprId, including wrapping it in parentheses, if /// needed. Otherwise, generates the original expression source. -/// FIXME: Identify smart-pointers as pointer types. Stencil maybeDeref(llvm::StringRef ExprId); /// Constructs an expression that idiomatically takes the address of the @@ -94,7 +93,6 @@ /// idiomatically takes the address of the expression bound to \p ExprId, /// including wrapping \p ExprId in parentheses, if needed. Otherwise, generates /// the original expression source. -/// FIXME: Identify smart-pointers as pointer types. Stencil maybeAddressOf(llvm::StringRef ExprId); /// Constructs a `MemberExpr` that accesses the named member (\p Member) of the diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp b/clang/lib/Tooling/Transformer/Stencil.cpp --- a/clang/lib/Tooling/Transformer/Stencil.cpp +++ b/clang/lib/Tooling/Transformer/Stencil.cpp @@ -195,6 +195,24 @@ return printNode(Data.Id, Match, Result); } +// FIXME: Consider memoizing this function using the `ASTContext`. +static bool isSmartPointerType(QualType Ty, ASTContext &Context) { + using namespace ::clang::ast_matchers; + + // Optimization: hard-code common smart-pointer types. This can/should be + // removed if we start caching the results of this function. + auto KnownSmartPointer = + cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr")); + const auto QuacksLikeASmartPointer = cxxRecordDecl( + hasMethod(cxxMethodDecl(hasOverloadedOperatorName("->"), + returns(qualType(pointsTo(type()))))), + hasMethod(cxxMethodDecl(hasOverloadedOperatorName("*"), + returns(qualType(references(type())))))); + const auto SmartPointer = qualType(hasDeclaration( + cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer)))); + return match(SmartPointer, Ty, Context).size() > 0; +} + Error evalData(const UnaryOperationData &Data, const MatchFinder::MatchResult &Match, std::string *Result) { // The `Describe` operation can be applied to any node, not just expressions, @@ -215,17 +233,37 @@ Source = tooling::buildDereference(*E, *Match.Context); break; case UnaryNodeOperator::MaybeDeref: - if (!E->getType()->isAnyPointerType()) { - *Result += tooling::getText(*E, *Match.Context); - return Error::success(); + if (E->getType()->isAnyPointerType() || + isSmartPointerType(E->getType(), *Match.Context)) { + // Strip off any operator->. This can only occur inside an actual arrow + // member access, so we treat it as equivalent to an actual object + // expression. + if (const auto *OpCall = dyn_cast(E)) { + if (OpCall->getOperator() == clang::OO_Arrow && + OpCall->getNumArgs() == 1) { + E = OpCall->getArg(0); + } + } + Source = tooling::buildDereference(*E, *Match.Context); + break; } - Source = tooling::buildDereference(*E, *Match.Context); - break; + *Result += tooling::getText(*E, *Match.Context); + return Error::success(); case UnaryNodeOperator::AddressOf: Source = tooling::buildAddressOf(*E, *Match.Context); break; case UnaryNodeOperator::MaybeAddressOf: - if (E->getType()->isAnyPointerType()) { + if (E->getType()->isAnyPointerType() || + isSmartPointerType(E->getType(), *Match.Context)) { + // Strip off any operator->. This can only occur inside an actual arrow + // member access, so we treat it as equivalent to an actual object + // expression. + if (const auto *OpCall = dyn_cast(E)) { + if (OpCall->getOperator() == clang::OO_Arrow && + OpCall->getNumArgs() == 1) { + E = OpCall->getArg(0); + } + } *Result += tooling::getText(*E, *Match.Context); return Error::success(); } diff --git a/clang/unittests/Tooling/StencilTest.cpp b/clang/unittests/Tooling/StencilTest.cpp --- a/clang/unittests/Tooling/StencilTest.cpp +++ b/clang/unittests/Tooling/StencilTest.cpp @@ -8,6 +8,7 @@ #include "clang/Tooling/Transformer/Stencil.h" #include "clang/AST/ASTTypeTraits.h" +#include "clang/AST/Expr.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Tooling/FixIt.h" #include "clang/Tooling/Tooling.h" @@ -29,10 +30,18 @@ using MatchResult = MatchFinder::MatchResult; // Create a valid translation-unit from a statement. -static std::string wrapSnippet(StringRef StatementCode) { - return ("namespace N { class C {}; } " - "namespace { class AnonC {}; } " - "struct S { int field; }; auto stencil_test_snippet = []{" + +static std::string wrapSnippet(StringRef ExtraPreface, + StringRef StatementCode) { + constexpr char Preface[] = R"cc( + namespace N { class C {}; } + namespace { class AnonC {}; } + struct S { int Field; }; + struct Smart { + S* operator->() const; + S& operator*() const; + }; + )cc"; + return (Preface + ExtraPreface + "auto stencil_test_snippet = []{" + StatementCode + "};") .str(); } @@ -55,10 +64,12 @@ // matcher correspondingly. `Matcher` should match one of the statements in // `StatementCode` exactly -- that is, produce exactly one match. However, // `StatementCode` may contain other statements not described by `Matcher`. +// `ExtraPreface` (optionally) adds extra decls to the TU, before the code. static llvm::Optional matchStmt(StringRef StatementCode, - StatementMatcher Matcher) { - auto AstUnit = tooling::buildASTFromCodeWithArgs(wrapSnippet(StatementCode), - {"-Wno-unused-value"}); + StatementMatcher Matcher, + StringRef ExtraPreface = "") { + auto AstUnit = tooling::buildASTFromCodeWithArgs( + wrapSnippet(ExtraPreface, StatementCode), {"-Wno-unused-value"}); if (AstUnit == nullptr) { ADD_FAILURE() << "AST construction failed"; return llvm::None; @@ -260,6 +271,42 @@ testExpr(Id, "int x; &x;", maybeDeref(Id), "x"); } +TEST_F(StencilTest, MaybeDerefSmartPointer) { + StringRef Id = "id"; + std::string Snippet = R"cc( + Smart x; + x; + )cc"; + testExpr(Id, Snippet, maybeDeref(Id), "*x"); +} + +// Tests that unique_ptr specifically is handled. +TEST_F(StencilTest, MaybeDerefSmartPointerUniquePtr) { + StringRef Id = "id"; + // We deliberately specify `unique_ptr` as empty to verify that it matches + // because of its name, rather than its contents. + StringRef ExtraPreface = + "namespace std { template class unique_ptr {}; }\n"; + StringRef Snippet = R"cc( + std::unique_ptr x; + x; + )cc"; + auto StmtMatch = matchStmt(Snippet, expr().bind(Id), ExtraPreface); + ASSERT_TRUE(StmtMatch); + EXPECT_THAT_EXPECTED(maybeDeref(Id)->eval(StmtMatch->Result), + HasValue(std::string("*x"))); +} + +TEST_F(StencilTest, MaybeDerefSmartPointerFromMemberExpr) { + StringRef Id = "id"; + std::string Snippet = "Smart x; x->Field;"; + auto StmtMatch = + matchStmt(Snippet, memberExpr(hasObjectExpression(expr().bind(Id)))); + ASSERT_TRUE(StmtMatch); + const Stencil Stencil = maybeDeref(Id); + EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result), HasValue("*x")); +} + TEST_F(StencilTest, MaybeAddressOfPointer) { StringRef Id = "id"; testExpr(Id, "int *x; x;", maybeAddressOf(Id), "x"); @@ -280,6 +327,26 @@ testExpr(Id, "int *x; *x;", addressOf(Id), "x"); } +TEST_F(StencilTest, MaybeAddressOfSmartPointer) { + StringRef Id = "id"; + testExpr(Id, "Smart x; x;", maybeAddressOf(Id), "x"); +} + +TEST_F(StencilTest, MaybeAddressOfSmartPointerFromMemberCall) { + StringRef Id = "id"; + std::string Snippet = "Smart x; x->Field;"; + auto StmtMatch = + matchStmt(Snippet, memberExpr(hasObjectExpression(expr().bind(Id)))); + ASSERT_TRUE(StmtMatch); + const Stencil Stencil = maybeAddressOf(Id); + EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result), HasValue("x")); +} + +TEST_F(StencilTest, MaybeAddressOfSmartPointerDerefNoCancel) { + StringRef Id = "id"; + testExpr(Id, "Smart x; *x;", maybeAddressOf(Id), "&*x"); +} + TEST_F(StencilTest, AccessOpValue) { StringRef Snippet = R"cc( S x;