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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
