dgoldman updated this revision to Diff 425644.
dgoldman marked 5 inline comments as done.
dgoldman added a comment.
- Don't use a method for computing the type
- Compute the type using printType
- Support MemberExprs and add a test for function pointers
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124486/new/
https://reviews.llvm.org/D124486
Files:
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -24,7 +24,7 @@
int z;
} t;
// return statement
- return [[[[t.b[[a]]r]](t.z)]];
+ return [[[[t.b[[a]]r]]([[t.z]])]];
}
void f() {
int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
@@ -59,11 +59,22 @@
EXPECT_AVAILABLE(AvailableCases);
ExtraArgs = {"-xc"};
- const char *AvailableButC = R"cpp(
+ const char *AvailableC = R"cpp(
void foo() {
int x = [[1]];
})cpp";
- EXPECT_UNAVAILABLE(AvailableButC);
+ EXPECT_AVAILABLE(AvailableC);
+ ExtraArgs = {"-xobjective-c"};
+ const char *AvailableObjC = R"cpp(
+ __attribute__((objc_root_class))
+ @interface Foo
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[1 + 2]];
+ }
+ @end)cpp";
+ EXPECT_AVAILABLE(AvailableObjC);
ExtraArgs = {};
const char *NoCrashCases = R"cpp(
@@ -82,7 +93,7 @@
int bar(int a = [[1]]);
int z = [[1]];
} t;
- return [[t]].bar([[[[t]].z]]);
+ return [[t]].bar([[t]].z);
}
void v() { return; }
// function default argument
@@ -123,7 +134,7 @@
EXPECT_UNAVAILABLE(UnavailableCases);
// vector of pairs of input and output strings
- const std::vector<std::pair<std::string, std::string>> InputOutputs = {
+ std::vector<std::pair<std::string, std::string>> InputOutputs = {
// extraction from variable declaration/assignment
{R"cpp(void varDecl() {
int a = 5 * (4 + (3 [[- 1)]]);
@@ -291,6 +302,92 @@
for (const auto &IO : InputOutputs) {
EXPECT_EQ(IO.second, apply(IO.first)) << IO.first;
}
+
+ ExtraArgs = {"-xobjective-c"};
+ EXPECT_UNAVAILABLE(R"cpp(
+ __attribute__((objc_root_class))
+ @interface Foo
+ - (void)setMethod1:(int)a;
+ - (int)method1;
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ [[self.method1]] = 1;
+ [[self.method1]] += 1;
+ [[self.prop1]] = 1;
+ [[self.prop1]] += 1;
+ }
+ @end)cpp");
+ InputOutputs = {
+ // Function Pointer
+ {R"cpp(struct Handlers {
+ void (*handlerFunc)(int);
+ };
+ void runFunction(void (*func)(int)) {}
+ void f(struct Handlers *handler) {
+ runFunction([[handler->handlerFunc]]);
+ })cpp",
+ R"cpp(struct Handlers {
+ void (*handlerFunc)(int);
+ };
+ void runFunction(void (*func)(int)) {}
+ void f(struct Handlers *handler) {
+ void (*placeholder)(int) = handler->handlerFunc; runFunction(placeholder);
+ })cpp"},
+ // We don't preserve types through typedefs.
+ {R"cpp(typedef long NSInteger;
+ void varDecl() {
+ NSInteger a = 2 * 5;
+ NSInteger b = [[a * 7]] + 3;
+ })cpp",
+ R"cpp(typedef long NSInteger;
+ void varDecl() {
+ NSInteger a = 2 * 5;
+ long placeholder = a * 7; NSInteger b = placeholder + 3;
+ })cpp"},
+ // Support ObjC property references (explicit property getter).
+ {R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[self.prop1]] + 1;
+ }
+ @end)cpp",
+ R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int placeholder = self.prop1; int x = placeholder + 1;
+ }
+ @end)cpp"},
+ // Support ObjC property references (implicit property getter).
+ {R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ - (int)method1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[self.method1]] + 1;
+ }
+ @end)cpp",
+ R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ - (int)method1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int placeholder = self.method1; int x = placeholder + 1;
+ }
+ @end)cpp"},
+ };
+ for (const auto &IO : InputOutputs) {
+ EXPECT_EQ(IO.second, apply(IO.first)) << IO.first;
+ }
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -5,6 +5,7 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
+#include "AST.h"
#include "ParsedAST.h"
#include "Protocol.h"
#include "Selection.h"
@@ -50,6 +51,7 @@
private:
bool Extractable = false;
const clang::Expr *Expr;
+ llvm::Optional<QualType> ExprType;
const SelectionTree::Node *ExprNode;
// Stmt before which we will extract
const clang::Stmt *InsertionPoint = nullptr;
@@ -61,6 +63,7 @@
bool exprIsValidOutside(const clang::Stmt *Scope) const;
// computes the Stmt before which we will extract out Expr
const clang::Stmt *computeInsertionPoint() const;
+ std::string computeExprType(const clang::Expr *E) const;
};
// Returns all the Decls referenced inside the given Expr
@@ -90,6 +93,32 @@
InsertionPoint = computeInsertionPoint();
if (InsertionPoint)
Extractable = true;
+ ExprType = Expr->getType();
+ // No need to fix up the type if we're just going to use "auto" later on.
+ if (Ctx.getLangOpts().CPlusPlus11)
+ return;
+ if (ExprType->getTypePtrOrNull() && Expr->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(Expr)) {
+ if (PR->isMessagingSetter()) {
+ // Don't support extracting a compound reference like `self.prop += 1`
+ // since the meaning changes after extraction since we'll no longer call
+ // the setter. Non compound access like `self.prop = 1` is invalid since
+ // it returns nil (setter method must have a void return type).
+ ExprType = llvm::None;
+ } else if (PR->isMessagingGetter()) {
+ if (PR->isExplicitProperty())
+ ExprType = PR->getExplicitProperty()->getType();
+ else
+ ExprType = PR->getImplicitPropertyGetter()->getReturnType();
+ }
+ } else {
+ ExprType = llvm::None;
+ }
+ }
+ if (!ExprType || !ExprType->getTypePtrOrNull())
+ Extractable = false;
+ else
+ AttributedType::stripOuterNullability(*ExprType);
}
// checks whether extracting before InsertionPoint will take a
@@ -173,9 +202,13 @@
toHalfOpenFileRange(SM, Ctx.getLangOpts(),
InsertionPoint->getSourceRange())
->getBegin();
- // FIXME: Replace auto with explicit type and add &/&& as necessary
- std::string ExtractedVarDecl = std::string("auto ") + VarName.str() + " = " +
- ExtractionCode.str() + "; ";
+ std::string ExtractedLHS;
+ if (Ctx.getLangOpts().CPlusPlus11)
+ ExtractedLHS = "auto " + VarName.str();
+ else
+ ExtractedLHS = printType(*ExprType, ExprNode->getDeclContext(), VarName);
+ std::string ExtractedVarDecl =
+ ExtractedLHS + " = " + ExtractionCode.str() + "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
}
@@ -365,13 +398,19 @@
return false;
// Void expressions can't be assigned to variables.
- if (const Type *ExprType = E->getType().getTypePtrOrNull())
- if (ExprType->isVoidType())
- return false;
+ const Type *ExprType = E->getType().getTypePtrOrNull();
+ if (ExprType && ExprType->isVoidType())
+ return false;
+
+ // Must know the type of the result in order to spell it, or instead use
+ // `auto` in C++.
+ if (!N->getDeclContext().getParentASTContext().getLangOpts().CPlusPlus11 &&
+ !ExprType)
+ return false;
// A plain reference to a name (e.g. variable) isn't worth extracting.
// FIXME: really? What if it's e.g. `std::is_same<void, void>::value`?
- if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+ if (llvm::isa<DeclRefExpr>(E))
return false;
// Extracting Exprs like a = 1 gives placeholder = a = 1 which isn't useful.
@@ -460,10 +499,6 @@
if (Inputs.SelectionBegin == Inputs.SelectionEnd)
return false;
const ASTContext &Ctx = Inputs.AST->getASTContext();
- // FIXME: Enable non-C++ cases once we start spelling types explicitly instead
- // of making use of auto.
- if (!Ctx.getLangOpts().CPlusPlus)
- return false;
const SourceManager &SM = Inputs.AST->getSourceManager();
if (const SelectionTree::Node *N =
computeExtractedExpr(Inputs.ASTSelection.commonAncestor()))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
