-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Comments
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. |
Not critical for 0.6, de-milestoning |
@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. |
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.)
|
@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. |
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) |
Is this issue still relevant today? |
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. |
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. |
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. |
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...
Global state isn't so desirable. The compiler stores an ident interner in local storage. It would be better to not do that.
The text was updated successfully, but these errors were encountered: