Page MenuHomePhabricator

[Clangd] Changed ExtractVariable to only work on non empty selections
ClosedPublic

Authored by SureYeaah on Jul 18 2019, 4:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

SureYeaah created this revision.Jul 18 2019, 4:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 4:22 AM
SureYeaah updated this revision to Diff 210526.Jul 18 2019, 4:29 AM

Added tests

kadircet added inline comments.Jul 18 2019, 4:45 AM
clang-tools-extra/clangd/refactor/Tweak.h
51 ↗(On Diff #210525)

maybe expose a SourceLocation SelectionEnd ?

SureYeaah marked an inline comment as done.Jul 18 2019, 5:49 AM
SureYeaah added inline comments.
clang-tools-extra/clangd/refactor/Tweak.h
51 ↗(On Diff #210525)

Should I also have a SelectionStart? Or just use Cursor?

SureYeaah updated this revision to Diff 210536.Jul 18 2019, 5:52 AM

Added selectionend

SureYeaah updated this revision to Diff 210537.Jul 18 2019, 5:53 AM

Fixed comment

kadircet added inline comments.Jul 18 2019, 5:57 AM
clang-tools-extra/clangd/refactor/Tweak.h
51 ↗(On Diff #210525)

I think it makes more sense to have SelectionBegin than Cursor, AFAIK we usually don't have a notion of Cursor` when provided with a selection.
It only makes sense for inputs coming from clients like vim, in which you only have a cursor rather then a selection. But I would like to know what @sammccall thinks about it as well.

sammccall added inline comments.Jul 18 2019, 8:05 AM
clang-tools-extra/clangd/refactor/Tweak.h
51 ↗(On Diff #210525)

I'd suggest unsigned SelectionStart, SelectionEnd as it more clearly communicates this is main-file only and can be easily used with Code.

I'd add a FIXME to remove Cursor as it's redundant (we can replace it with a method)

SureYeaah updated this revision to Diff 210577.Jul 18 2019, 8:16 AM

Added SelectionBegin and SelectionEnd to Tweak::Selection

SureYeaah marked an inline comment as done.Jul 18 2019, 8:16 AM
sammccall accepted this revision.Jul 18 2019, 8:21 AM
This revision is now accepted and ready to land.Jul 18 2019, 8:21 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 8:39 AM