[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:210
+//  - we don't look inside macro expansions in the subexpressions
+//  - we only adjust the extracted range, so references in the unselected parts
+//    of the AST expression (e.g. `a`) are still considered referenced for
----------------
A related FIXME would be helpful since it's pretty easy to fix(I think?) and seems like something important.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:213
+//    the purposes of calculating the insertion point.
+namespace {
+
----------------
Why do we need an anon namespace inside another anon namespace?


================
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;
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:235
+                N.ASTNode.get<Expr>())) {
+      if (Op->isInfixBinaryOp()) {
+        Kind = BinaryOperator::getOverloadedOpcode(Op->getOperator());
----------------
Can we negate this if to reduce nesting?


================
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) {
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:393
   std::string VarName = "dummy";
+  SourceRange Range = Target->getExtractionChars();
   // insert new variable declaration
----------------
Why not just compute this inside ExtractionContext?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65139