-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DSLX:LS] Add support for prepareRename and rename #1640
base: main
Are you sure you want to change the base?
[DSLX:LS] Add support for prepareRename and rename #1640
Conversation
bbe3eb5
to
424a5f1
Compare
Mind syncing & rerunning? Looks like CI failed... |
bac0775
to
4c84049
Compare
Module-scoped renames for non-pub members.
4c84049
to
acac707
Compare
Yeah, was really change related, had trouble seeing it since baseline was red. Setting parent link on module-top AST nodes was hazardous for some reason I don't fully understand so I made a simpler change. PTAL, thanks. |
xls/dslx/lsp/find_definition.h
Outdated
@@ -41,7 +41,8 @@ namespace xls::dslx { | |||
// colon-reference to a construct in another module will return nullopt. | |||
std::optional<Span> FindDefinition(const Module& m, const Pos& selected, | |||
const TypeInfo& type_info, | |||
ImportData& import_data); | |||
ImportData& import_data, | |||
const NameDef** name_def_out = nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than returning name_def_out through an argument, it seems easier to have this be std::optional<NameDef*> FindDefinition(...)
and have existing usages update to get the span from the NameDef
.
What do you think? Is it really annoying to update the usages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done, PTAL.
Works for intra-function and intra-module definitions.
Support is not present for cross-workspace renames yet.