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

Make the 'a lifetime on TyCtxt useless #56601

Merged
merged 2 commits into from Dec 19, 2018

Conversation

Projects
None yet
6 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 7, 2018

cc @rust-lang/compiler

r? @nikomatsakis

@eddyb

eddyb approved these changes Dec 7, 2018

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

I love the idea of simplifying the TyCtxt setup!

Show resolved Hide resolved src/librustc/infer/mod.rs
@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 10, 2018

Would we be able to get rid of 'a entirely in a subsequent change? That would be awesome!

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 10, 2018

@michaelwoerister Yes.

I also propose a rename to this:

type QueryCx<'qx> = TyCtxt<'qx, 'qx, 'qx>;
type LocalCx<'qx, 'lx> = TyCtxt<'lx, 'qx, 'lx>;
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 10, 2018

@Zoxc Note that pretty much all APIs must take the more general version ("LocalCx"), as to avoid callers having to call (the equivalent of) .global_tcx().

Because of that, I don't consider LocalCx "local" but rather "potentially-local", so it's more that the other one is the "global" context.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 11, 2018

I like GlobalCx and QueryCx -- I personally would prefer the lifetimes to be ordered "small to large", so QueryCx<'local, 'global> and GlobalCx<'global>. But anyway that's for after this PR, I suppose.

{
let interners = CtxtInterners::new(arena);
*interners = Some(CtxtInterners::new(&arena));

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 11, 2018

Contributor

Maybe we should assert that this is None and set it back to None before we return, then?

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 11, 2018

Contributor

I guess it hardly matters, it's just the interning maps.

@@ -1604,20 +1611,23 @@ impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
}
}

impl<'gcx: 'tcx, 'tcx> GlobalCtxt<'gcx> {
impl<'gcx> GlobalCtxt<'gcx> {
/// Call the closure with a local `TyCtxt` using the given arena.

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 11, 2018

Contributor

Can we add a comment here:


The interners parameter is a reference to a storage slot with 'tcx lifetime that we can use. It is expected to be None on entry and will be reset to None on return. This allows us to create a &'tcx reference to the interners while the tcx is active.


that presumes, of course, that you accept my proposal above =)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 11, 2018

In any case, I'm 👍 on this change, I'd like some kind of comment to make the role of the Option a bit clearer, primarily. Not sure where is the best place to put it though.

@Zoxc Zoxc force-pushed the Zoxc:lifetime-killer branch from 2fb0acf to d0190d3 Dec 13, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 17, 2018

@bors r+

Good enough

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

📌 Commit d0190d3 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

⌛️ Testing commit d0190d3 with merge 0a4a4ff...

bors added a commit that referenced this pull request Dec 19, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 0a4a4ff to master...

@bors bors merged commit d0190d3 into rust-lang:master Dec 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Zoxc Zoxc deleted the Zoxc:lifetime-killer branch Dec 19, 2018

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 24, 2018

I have found two drawbacks to doing this:

  • GlobalCtxt's destructor may not access references with the 'tcx lifetime. So we need to use #[may_dangle] on Drop impls.
  • After creating the GlobalCtxt we can no longer gain ownership or even mutable access of it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment