Skip to content

Rewrite TLS to require less usage of @ #7677

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

Merged
merged 8 commits into from
Jul 12, 2013
Merged

Conversation

alexcrichton
Copy link
Member

cc #6004 and #3273

This is a rewrite of TLS to get towards not requiring @ when using task local storage. Most of the rewrite is straightforward, although there are two caveats:

  1. Changing local_set to not require @ is blocked on Polymorphically creating traits barely works #7673
  2. The code in local_pop is some of the most unsafe code I've written. A second set of eyes should definitely scrutinize it...

The public-facing interface currently hasn't changed, although it will have to change because local_data::get cannot return Option<T>, nor can it return Option<&T> (the lifetime isn't known). This will have to be changed to be given a closure which yield &T (or as an Option). I didn't do this part of the api rewrite in this pull request as I figured that it could wait until when @ is fully removed.

This also doesn't deal with the issue of using something other than functions as keys, but I'm looking into using static slices (as mentioned in the issues).

@graydon
Copy link
Contributor

graydon commented Jul 10, 2013

\o/

@alexcrichton
Copy link
Member Author

After @nikomatsakis's comment on #7673, it actually turns out that I can work around the compiler right now in a bit of an odd way to get TLS to not require @ at all. Unfortunately, it also requires a new snapshot (@Aatch 😃).

@alexcrichton
Copy link
Member Author

Turns out I was able to work around everything completely (with excessive use of stage0), so this now completely removes all requirements of @.

Things also changed a fair amount from previously. I'd definitely appreciate someone reading the comments to make sure that I'm still sane.

@alexcrichton
Copy link
Member Author

One unfortunate side effect is that for now check-stage1-std will not pass (two different TLS versions don't play nice with one another). This would be fixed with a snapshot.

bors added a commit that referenced this pull request Jul 12, 2013
cc #6004 and #3273

This is a rewrite of TLS to get towards not requiring `@` when using task local storage. Most of the rewrite is straightforward, although there are two caveats:

1. Changing `local_set` to not require `@` is blocked on #7673
2. The code in `local_pop` is some of the most unsafe code I've written. A second set of eyes should definitely scrutinize it...

The public-facing interface currently hasn't changed, although it will have to change because `local_data::get` cannot return `Option<T>`, nor can it return `Option<&T>` (the lifetime isn't known). This will have to be changed to be given a closure which yield `&T` (or as an Option). I didn't do this part of the api rewrite in this pull request as I figured that it could wait until when `@` is fully removed.

This also doesn't deal with the issue of using something other than functions as keys, but I'm looking into using static slices (as mentioned in the issues).
@bors bors closed this Jul 12, 2013
@bors bors merged commit a15c1b4 into rust-lang:master Jul 12, 2013
@graydon
Copy link
Contributor

graydon commented Jul 12, 2013

Awesome, thanks

@alexcrichton alexcrichton deleted the tls-gc branch July 12, 2013 07:30
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 7, 2021
…msteffen

fix bug for large_enum_variants

Fix the discussion problem in the issue of rust-lang/rust-clippy#7666 (comment)

About the false positive problem of case:
```rust
enum LargeEnum6 {
    A,
    B([u8;255]),
    C([u8;200]),
}
```

changelog: Fix largest_enum_variant wrongly identifying the second largest variant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants