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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:213
+//    the purposes of calculating the insertion point.
+namespace {
+
----------------
SureYeaah wrote:
> Why do we need an anon namespace inside another anon namespace?
Oops, forgot we already were in one.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:217
+// It can represent either an overloaded or built-in operator.
+struct ParsedBinaryOperator {
+  BinaryOperatorKind Kind;
----------------
SureYeaah wrote:
> Wouldn't it make more sense this to have this in some other place (maybe Selection) because other refactorings might want to make use of this feature?
It's possible, but I think we should cross that bridge when we come to it, because it's not obvious what those other refactorings are.

There might be some subtle ways in which I've specialized the code for this use case, and it's easier to see that and polish it when we have more use cases in hand.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:283
+const SelectionTree::Node *getEndpoint(const SelectionTree::Node &N,
+                                       BinaryOperatorKind OuterOp, bool IsStart,
+                                       const SourceManager &SM) {
----------------
SureYeaah wrote:
> We're considering the case where all the operators are the same. Isn't it true that in that case, the End will be the the first 'selected RHS' that we find during the traversal? If so, can't we just get rid of the IsStart?
Oh, right - I'm assuming a much more general tree than can actually exist.
This code is trying to handle an arbitrary binary tree of binops, but of course each actual operator is either left-associative or right-associative, and so it's a linked-list-shaped binary tree.
And the actual code is pretty simple, but the explanation sure is complicated.

And in fact, all the operators are left-associative! I didn't really want to hard-code that, but it'll simplify the code a lot I think.

So this should probably just be a simple loop:
```
findEndpoints(L, R):
  while (LL, RL) = parseBinop(L):
    L = (LL is selected) ? LL : RL
  return (L, R)
```

I'll rewrite this tomorrow. Thanks for the hint!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65139