[PATCH] D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.
Herald added a subscriber: llvm-commits.

In D57570#1380354 <https://reviews.llvm.org/D57570#1380354>, @ilya-biryukov wrote:

> In D57570#1380327 <https://reviews.llvm.org/D57570#1380327>, @sammccall wrote:
>
> > > Maybe we should consider stopping the traversals up at some points, e.g. at lambda decls or something similar to limit this class of problems in the future?
> >
> > If you mean in some frameworky sort of way... yeah, it's quite likely we need something. At least to make the Node* -> DynTypedNode -> Stmt -> IfStmt dance less clunky.
> >  I've deliberately avoided adding too much in the way of helpers for now in favor of the raw data structures, mostly wanting to see what the common patterns are - we can afford to write a few of these by hand.
>
>
> Exactly, the code dealing with typed nodes there is definitely too verbose. Thinking about helpers like `findAncestor<IfStmt>()`.
>  And yeah, totally fine to keep it as is for now, it's great that the change is focused.


e.g. `findAncestor` is nice but here we want to walk up but break if we hit a limit node (e.g. compoundstmt).
If that turns out to be the normal case, then a plain `findAncestor` isn't such a useful primitive.
Some other thoughts:

- In a draft I did have a tree traversal where the node visitor could return {continue, abort, prune} - seemed cool but I didn't actually have a use for it
- just some direct casting operators (`Node->as<IfStmt>()`) would help, probably.

> Another concern that came to mind: what should we do if two if statements are selected? IIUC the commonAncestor would be their parent and we don't have the action available, right?

Correct. Ranges are confusing, I think we need to mostly treat them as "a target to perform an action on" rather than "a selection of targets to perform actions on". Maybe some exceptions like maybe bulk actions on includes or member function decls, but the simple model should work for most tweaks.

> Could we add a test for this case?

Done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57570