[PATCH] D65139: [clangd] Support extraction of binary "subexpressions" like a + [[b + c]].

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:50
   // Generate Replacement for declaring the selected Expr as a new variable
-  tooling::Replacement insertDeclaration(llvm::StringRef VarName) const;
+  tooling::Replacement insertDeclaration(llvm::StringRef VarName,
+                                         SourceRange InitChars) const;
----------------
Nit: same parameter order for replaceWithVar and insertDeclaration.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:245
+        assert(E && "callee and args should be Exprs!");
+        if (E == Op->getArg(0) || E == Op->getArg(1))
+          SelectedOperands.push_back(Child);
----------------
Why do we need to check this for CXXOperatorCallExpr and not BinaryOperator?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:305
+  const SelectionTree::Node *Start = Op.SelectedOperands.front(); // LHS
+  const SelectionTree::Node *End = Op.SelectedOperands.back();    // RHS
+  // End is already correct, Start needs to be pushed down to the right spot.
----------------
For 1 + [[2 + 3 * 4]] + 5, we probably don't get an invalid range from this function. If so, we might want to add one more check where we parse End and compare it's operator.




================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:314
+    }
+    // Op.LHS (partially) selected, so descend into it.
+    Start = Op.SelectedOperands.front();
----------------
Nit: For [[a + b + c]],

       +
     /  \
    +    c
   / \
  a   b

For the second + as Op, a would be completely selected. So Op.LHS can be partially or completely selected.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:448
+
+          // Binary subexpressions
+          {R"cpp(void f() {
----------------
Can we have some tests with macros as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65139