dgoldman created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
dgoldman requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
- Use the expression's type for non-C++ as the variable type. This works well,
but might not preserve the typedefs due to type canonicalization.
- Improve support for Objective-C property references which are represented
using `ObjCPropertyRefExpr` and `BuiltinType::PseudoObject`.
Repository:
rG LLVM Github Monorepo
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
@@ -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(
@@ -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,78 @@
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 = {
+ // We don't preserve types through typedefs.
+ // FIXME: Ideally we could preserve the non-canonical type.
+ {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
@@ -50,6 +50,7 @@
private:
bool Extractable = false;
const clang::Expr *Expr;
+ std::string Type;
const SelectionTree::Node *ExprNode;
// Stmt before which we will extract
const clang::Stmt *InsertionPoint = nullptr;
@@ -61,6 +62,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 +92,9 @@
InsertionPoint = computeInsertionPoint();
if (InsertionPoint)
Extractable = true;
+ Type = computeExprType(Expr);
+ if (Type.empty())
+ Extractable = false;
}
// checks whether extracting before InsertionPoint will take a
@@ -156,6 +161,33 @@
return nullptr;
}
+std::string
+ExtractionContext::computeExprType(const clang::Expr *E) const {
+ if (Ctx.getLangOpts().CPlusPlus) {
+ return "auto";
+ }
+ QualType ExprType = E->getType();
+ if (E->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(E)) {
+ 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).
+ return "";
+ } else if (PR->isMessagingGetter()) {
+ if (PR->isExplicitProperty())
+ ExprType = PR->getExplicitProperty()->getType();
+ else
+ ExprType = PR->getImplicitPropertyGetter()->getReturnType();
+ }
+ }
+ }
+ // Avoid including outer nullability on local variables.
+ AttributedType::stripOuterNullability(ExprType);
+ return ExprType.getAsString();
+}
+
// returns the replacement for substituting the extraction with VarName
tooling::Replacement
ExtractionContext::replaceWithVar(SourceRange Chars,
@@ -173,9 +205,8 @@
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 ExtractedVarDecl =
+ Type + " " + VarName.str() + " = " + ExtractionCode.str() + "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
}
@@ -365,9 +396,15 @@
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().CPlusPlus &&
+ !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`?
@@ -460,10 +497,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
