Skip to content

Stop putting the ident interner in local storage #3738

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

Closed
brson opened this issue Oct 12, 2012 · 11 comments
Closed

Stop putting the ident interner in local storage #3738

brson opened this issue Oct 12, 2012 · 11 comments
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

@brson
Copy link
Contributor

brson commented Oct 12, 2012

Global state isn't so desirable. The compiler stores an ident interner in local storage. It would be better to not do that.

@nikomatsakis
Copy link
Contributor

This is dependent on @erickt's work on allowing serialization of a type to be specialized to a particular kind of serializer. Unfortunately, that is hitting an ICE. I don't think there's a bug for this, I'll open one.

@catamorphism
Copy link
Contributor

Not critical for 0.6, de-milestoning

@jbclements
Copy link
Contributor

@paulstansifer put the interner in TLS after giving up on passing it around everywhere. After looking at code on both sides of this divide, I feel like the TLS approach is frankly a lot simpler; less code everywhere, less chance of linked pieces of code accidentally using different interners. In my mind, the right way to manage the size of the interner is to have a dynamic parameterize-like mechanism for setting and clearing the interner.

So, personally, I would make this a WONTFIX.

@pnkfelix
Copy link
Member

visiting for triage, email from 2013-09-02.

(Closing as wont fix sounds tempting. I want to investigate a little more myself before doing so.)

aside: @jbclements when you wrote "less code of linked pieces ...", did you mean "less chance of ..." ?

@jbclements
Copy link
Contributor

@pnkfelix: yes, absolutely. Er, I mean: what are you talking about? I don't see that text in my comment. :)

Related: I really think this is a bit of a research question: seems like there are lots of times where you want lexical scope, but you don't want a lexical var cluttering up every arg list, and indeed, you may have the same value coming in from two different directions, and want to ensure that the two are the same (as with the interner, here). Let me know if this doesn't make sense.

@nikomatsakis
Copy link
Contributor

I am sympathetic to @jbclements's point, but it's also true that threading state around explicitly has a lot of concrete benefits. It makes it very easy to analyze what a function reads and affects -- and I mean this both automatically and for the human user. I guess for my taste I've found careful use of methods to generally be satisfactory. Still, I'm not sure whether this bug is worth fixing or not, I can't say I've looked too closely at the code. Certainly having the interner in TLS is nice when debugging (and the type context would be nice too...)

(As an aside, scala is the only language I know that attempts to provide something other than TLS vs explicit parameters, with its implicit parameter lists)

@steveklabnik
Copy link
Member

Is this issue still relevant today?

@steveklabnik
Copy link
Member

Triage: it's been a year and a half since I asked about relevance; I'm going to close. A four-digit bug! whew.

Please let me know if this is wrong.

@jonas-schievink
Copy link
Contributor

@steveklabnik steveklabnik reopened this Jan 3, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for compiler team. I'm hoping that we can assign someone to investigate and then close this. Looking at the code myself, I'm inclined to say that fixing this isn't worth it.

@Mark-Simulacrum Mark-Simulacrum added I-nominated 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. and removed A-frontend Area: Compiler frontend (errors, parsing and HIR) labels Jun 23, 2017
@nikomatsakis
Copy link
Contributor

I agree. Let's close this. Maybe we will do the refactoring and maybe we won't, but it's not worth keeping this issue open right now.

RalfJung pushed a commit to RalfJung/rust that referenced this issue Jul 14, 2024
Stacked Borrows: fix PartialEq for Stack

We have to compare unknown_bottom as well, it is semantically relevant! This could have merged two adjacent stacks that are not actually equal...
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

9 participants