[PATCH] D64717: [Clangd] Fixed ExtractVariable for MemberExprs and Assignment Exprs

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 209853.
SureYeaah marked 2 inline comments as done.
SureYeaah added a comment.

Added comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64717/new/

https://reviews.llvm.org/D64717

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -299,10 +299,10 @@
       return ^1;
     }
     void f() {
-      int a = 5 + [[4 ^* ^xyz^()]];
+      int a = 5 + [[4 ^* xyz^()]];
       // multivariable initialization
       if(1)
-        int x = ^1, y = ^a + 1, a = ^1, z = a + 1;
+        int x = ^1, y = [[a + 1]], a = ^1, z = a + 1;
       // if without else
       if(^1) {}
       // if with else
@@ -320,7 +320,7 @@
         a = ^2;
       // while 
       while(a < ^1)
-        ^a++;
+        [[a++]];
       // do while 
       do
         a = ^1;
@@ -340,8 +340,10 @@
       return 1;
       class T {
         T(int a = ^1) {};
+        T f() { return T(); }
         int xyz = ^1;
       };
+      [[T.[[^t]]]]();
     }
     // function default argument
     void f(int b = ^1) {
@@ -359,6 +361,10 @@
         a = ^a ^+ 1;
       // lambda 
       auto lamb = [&^a, &^b](int r = ^1) {return 1;}
+      // assigment 
+      [[a ^= 5]];
+      // DeclRefExpr
+      a = [[b]], b = [[xyz]]();
     }
   )cpp");
   // vector of pairs of input and output strings
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
@@ -77,16 +77,25 @@
   return Visitor.ReferencedDecls;
 }
 
-// An expr is not extractable if it's null or an expression of type void
-// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a =
+// Decide which types of Exprs are allowed for extraction
 static bool isExtractableExpr(const clang::Expr *Expr) {
-  if (Expr) {
-    const Type *ExprType = Expr->getType().getTypePtrOrNull();
-    // FIXME: check if we need to cover any other types
-    if (ExprType)
-      return !ExprType->isVoidType();
-  }
-  return false;
+  if (!Expr)
+    return false;
+  // Expressions with type void can't be assigned.
+  // FIXME: check if we need to cover any other types
+  if (const Type *ExprType = Expr->getType().getTypePtrOrNull())
+    if (ExprType->isVoidType())
+      return false;
+  // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
+  if (const BinaryOperator *BinOpExpr =
+          llvm::dyn_cast_or_null<BinaryOperator>(Expr))
+    if (BinOpExpr->getOpcode() == BinaryOperatorKind::BO_Assign)
+      return false;
+  // Extracting just a = [[b]] or a = [[foo]]() isn't that useful.
+  // Extracting [[Foo.bar]]() is neither legal nor useful.
+  if (llvm::isa<DeclRefExpr>(Expr) || llvm::isa<MemberExpr>(Expr))
+    return false;
+  return true;
 }
 
 ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
@@ -124,6 +133,7 @@
 
 // FIXME: Extraction from switch and case statements
 // FIXME: Doens't work for FoldExpr
+// FIXME: Ensure extraction from loops doesn't change semantics
 const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
   // returns true if we can extract before InsertionPoint
   auto CanExtractOutside =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64717.209853.patch
Type: text/x-patch
Size: 3237 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190715/836484b3/attachment.bin>