Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve SemIR naming of import_refs #4769

Closed
4 tasks
zygoloid opened this issue Jan 7, 2025 · 5 comments · Fixed by #4824
Closed
4 tasks

Improve SemIR naming of import_refs #4769

zygoloid opened this issue Jan 7, 2025 · 5 comments · Fixed by #4824
Labels
good first issue Possibly a good first issue for newcomers toolchain

Comments

@zygoloid
Copy link
Contributor

zygoloid commented Jan 7, 2025 •
edited
Loading

Currently import_refs have a non-descriptive name, containing only a content hash and disambiguator, such as %import_ref.a69 or %import_ref.f9d349.2. We should be able to do better.

Ideas:

  • Include the name of the package and library from which the entity was ultimately imported in the name. We may want to do some shortening here, for example we could take the component of the library name following the final /.
  • When the import_ref is loaded, look at its constant value and use that to form a name for the import_ref.
  • Use the instruction in the target IR to form a name.
  • Cache InstNamers for all IRs being processed by the toolchain invocation, and actually use the name of the instruction (including disambiguator) from the target IR, either in the name of the import_ref or at least in the right-hand side when printing formatted SemIR.
@zygoloid zygoloid added good first issue Possibly a good first issue for newcomers toolchain labels Jan 7, 2025
@nirmal-j-patel
Copy link
Contributor

I recently submitted a pull request to fix a devcontainer issue. However, I have not taken any issues related to toolchain yet. Would it be ok if I attempt this?

@zygoloid
Copy link
Contributor Author

I recently submitted a pull request to fix a devcontainer issue. However, I have not taken any issues related to toolchain yet. Would it be ok if I attempt this?

Yes, please go ahead. Our policy is to not assign "good first issue" issues, so please feel free to get started.

@gamestime102

This comment was marked as off-topic.

Sign in to view
@chandlerc
Copy link
Contributor

@gamestime102 - If you'd like to submit a patch, please use a GitHub pull request. Thanks!

@gamestime102
Copy link

thank you

@clavin clavin mentioned this issue Jan 20, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 22, 2025
@clavin
Improve SemIR naming of import_refs (#4824)
3f4de65 
Changes the name of SemIR `import_ref`s to use the format
`<package>.<entity>`.

<table>
<tr><th>Before</th><th>After</th></tr>
<tr>
<td><code>%import_ref.05a: type</code></td>
<td><code>%Main.D: type</code></td>
</tr>
<tr>
<td><code>%import_ref.8f2: &lt;witness&gt;</code></td>
<td><code>%Main.import_ref.8f2: &lt;witness&gt;</code></td>
</tr>
</table>

* [Discord discussion in
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1330253540999827577)
* Closes #4769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Possibly a good first issue for newcomers toolchain
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve SemIR naming of import_refs clavin/carbon-lang
4 participants
@zygoloid @chandlerc @nirmal-j-patel @gamestime102