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.

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

Change Symbol::as_str to &self -> &str #83295

Closed
oli-obk opened this issue Mar 19, 2021 · 1 comment
Closed

Change Symbol::as_str to &self -> &str #83295

oli-obk opened this issue Mar 19, 2021 · 1 comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 19, 2021

#76460 experimented with this, but it was never finished. We should figure out whether we actually want this

Pros:

  • &str is nicer to work with than SymbolStr, e.g. avoids the need for some & and &* occurrences.
  • It would be nice to eliminate SymbolStr.
  • It avoids the lie that SymbolStr strings have a static lifetime, replacing it with a smaller lie that the strings have a lifetime tied to the lifetime of the input Symbol.

Cons:

  • That lifetime is still a lie, and it's possible to cause crashes with this.
  • The lie is less obvious in a vanilla &str than it is within a SymbolStr.
  • We can't mark &str as !Send and !Sync.
    • this may be why SymbolStr (or what it was called previously) was introduced in the first place, because Zoxc did it together with the work on compiler parallelization.
  • I haven't yet eliminated SymbolStr entirely. The to_stable_hash_key() is the hardest case.
@oli-obk oli-obk added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Mar 19, 2021
@Kobzol
Copy link
Contributor

Kobzol commented Dec 28, 2021

I supposed that this was fixed by #91957?

@oli-obk oli-obk closed this as completed Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants