[PATCH] D64717: [Clangd] Fixed ExtractVariable for MemberExprs and Assignment Exprs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D64717#1585682 <https://reviews.llvm.org/D64717#1585682>, @SureYeaah wrote:

> In D64717#1585632 <https://reviews.llvm.org/D64717#1585632>, @sammccall wrote:
>
> > In D64717#1585542 <https://reviews.llvm.org/D64717#1585542>, @SureYeaah wrote:
> >
> > > In D64717#1585512 <https://reviews.llvm.org/D64717#1585512>, @sammccall wrote:
> > >
> > > > Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?
> > > >
> > > > E.g. if `bar(1,2,3, f[[o]]o(4,5));` seems like it should extract the call too `foo(4,5)`, not fail to trigger entirely.
> > >
> > >
> > > Selecting `f[[o]]o(4,5)` will just extract the `foo` which would be a `DeclRefExpr`. We want to extract the `CallExpr` for which we would need to select the brackets as well.
> >
> >
> > I agree, but this patch doesn't do that. (I think?)
> >  Instead it bans extraction entirely in this case.
>
>
> Yes it doesn't. Our current model doesn't extend the user selection and as such we only check the commonAncestor of whatever the user has selected. Should this be changed then?


Yes I think you want to walk up the tree like you do for finding the target stmt, giving up when you hit a non-expr.

This didn't matter prior to this patch as void subexprs are extremely rare. But declrefs are not!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64717