[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;
----------------
sammccall wrote:
> 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?
Sorry yes, makes sense.


================
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.
----------------
sammccall wrote:
> 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).
> 
But isn't the tree something like

                    +
                 /    \ 
                +     5
             /     \
         +           *
       /  \       /   \
       1    2    3    4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65139