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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 8 inline comments as done.
sammccall 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;
----------------
SureYeaah wrote:
> Nit: same parameter order for replaceWithVar and insertDeclaration.
I think this *is* the same parameter order semantically: this is roughly (thing, definition) in both cases.
The fact that the *types* match but in the opposite order is just a coincidence I think?


================
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);
----------------
SureYeaah wrote:
> Why do we need to check this for CXXOperatorCallExpr and not BinaryOperator?
Because there's no child AST node corresponding to a builtin `+`, but there is one corresponding to an overloaded `+`: a `DeclRefExpr` to an `operator+` FunctionDecl.


================
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.
----------------
SureYeaah wrote:
> 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.
> 
> 
I'm not quite sure what you mean here.

Annotating the operators, for `1 +a [[2 +b 3 *c 4]] +d 5`:
 - N is `*c`
 - RHS is `4`
 - LHS is `1 +a 2 +b 3 *c 4`.

The point is that RHS can never be an operator of the same type, because if we replaced RHS with `x +e y` then N would be that `+e` instead (since `+` is left-associative).



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:314
+    }
+    // Op.LHS (partially) selected, so descend into it.
+    Start = Op.SelectedOperands.front();
----------------
SureYeaah wrote:
> 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.
Yeah, I meant "at least partially". Elaborated comment.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:448
+
+          // Binary subexpressions
+          {R"cpp(void f() {
----------------
SureYeaah wrote:
> Can we have some tests with macros as well?
Added a simple one that just verifies we support operands being wrapped in macros, but not operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65139