sammccall marked an inline comment as done. sammccall added inline comments.
================ 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: > 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 Whoops, somehow I transcribed that without noticing c was a * instead of a +. From offline discussion: the code is apparently correct here: RHS can't be a matching operator, because it violates associativity: the parse tree would be rotated in that case. We don't care to distinguish between mismatching operator vs some other node entirely. - Expanded a comment to cover this more explicitly - adjusted a testcase to ensure that we expand a `+`-selection across a `*` on the RHS as well as the LHS. There's also the idea that it would be nice to verify that we don't bail out of this function just because the selection covers some mismatched operator. The idea of a test that we keep digging right if left mismatches and vice versa is appealing, but as described we never need to to dig right, so such a case doesn't exist. I've settled for ensuring that 0 + 1 * [2 + 3] * 4 + 5 expands to cover 1 and 4, but not 0 or 5. 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
