Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

rustc_trans: reorganize CrateContext and rename context types. #47209

Merged
merged 7 commits into from
Jan 16, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jan 5, 2018

Firstly, the {Shared,Local}CrateContext hasn't been meaningful for a while now, and this PR resolves it by moving all their fields to CrateContext and removing redundant accessor methods.

Secondly, this PR contains the following mass-renames:

  • ccx: CrateContext -> cx: CodegenCx
  • mircx: MirContext -> fx: FunctionCx
  • bcx: Builder -> bx: Builder

r? @nikomatsakis

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2018
@petrochenkov
Copy link
Contributor

Some notes:

fx: FunctionCx

Type checking has commonly used and very similarly named fcx: FnCtxt.

ccx -> cx, bcx -> bx

tcx and fcx are still three-letter. Inconsistency! :)

@eddyb
Copy link
Member Author

eddyb commented Jan 5, 2018

So the divergence is intentional but I'm not sure yet about tx: Ty(pe)Cx to propose doing this everywhere already.

@nikomatsakis
Copy link
Contributor

I'm not entirely sure how I feel about bx and fx and so forth, but I'm not really opposed. I guess if we're going for obscure, might as well be really obscure?

cc @rust-lang/compiler -- anybody else care to leave their 2 cents?

@eddyb
Copy link
Member Author

eddyb commented Jan 5, 2018

My line of reasoning is that using one letter with a cx suffix everywhere isn't more useful than a shorter suffix, as long as it's an unambiguous convention (which I assume x could be?).
For the context types, I also think Context or Ctxt is the least relevant part of them.

Hence, fx: FunctionCx (although in Rust, FnCx could also work). Like I said, I'm not ready to propose tx: Ty(pe)Cx, but if nobody hates it, it's on the table.

@nikomatsakis
Copy link
Contributor

@eddyb I think I agree with that reasoning.

@bors
Copy link
Contributor

bors commented Jan 6, 2018

☔ The latest upstream changes (presumably #47225) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jan 6, 2018

☔ The latest upstream changes (presumably #47235) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

I think that ccx, fcx, and bcx work better as memnonics. But I don't have a strong opinion on it.

@nikomatsakis
Copy link
Contributor

It does seem like bcx, fcx, and so forth make it more obvious that there is a pattern. That is, fx and bx are not obviously related to one another -- they could just be independent fields whose names happen to end in x. Still, both are pretty opaque.

@nikomatsakis
Copy link
Contributor

Having let this sit for a bit, I feel like I'm leaning mildly against the renaming of fcx and friends to fx. It just feels like a step backwards in clarity to me for not much gain.

@nikomatsakis
Copy link
Contributor

@eddyb and I were chatting on IRC and discussing the full set of names. I feel like this change wants to take all of them into account. One thought we had was that the name "tcx", while it has a lot of history in it, isn't especially meaningful -- and as we move to more and more querification, it will become increasingly inappropriate (because it's the context for more than types). We were thinking that "query context" would be a better name, and that then one might write just qx and Qx for the type (not sure what the lifetime names would be? 'qx and 'gx?). That seems like a pretty unique combination of letters. In that setting, though, I feel like things like bx and fx are more obvious. (I suppose the alternative would be to use qcx, though.)

@nikomatsakis
Copy link
Contributor

@michaelwoerister @pnkfelix @petrochenkov thoughts on previous comment?

@petrochenkov
Copy link
Contributor

@nikomatsakis
No thoughts :)

@michaelwoerister
Copy link
Member

I like qcx, I'm OK with qx.

@nikomatsakis
Copy link
Contributor

OK. I think I'm tired of debating about this. @eddyb do what you feel is best =)

@bors
Copy link
Contributor

bors commented Jan 14, 2018

☔ The latest upstream changes (presumably #47223) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented Jan 15, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 15, 2018

📌 Commit 4e40a0d has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 16, 2018

⌛ Testing commit 4e40a0d with merge da569fa...

bors added a commit that referenced this pull request Jan 16, 2018
rustc_trans: reorganize CrateContext and rename context types.

Firstly, the `{Shared,Local}CrateContext` hasn't been meaningful for a while now, and this PR resolves it by moving all their fields to `CrateContext` and removing redundant accessor methods.

Secondly, this PR contains the following mass-renames:
* `ccx: CrateContext` -> `cx: CodegenCx`
* `mircx: MirContext` -> `fx: FunctionCx`
* `bcx: Builder` -> `bx: Builder`

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jan 16, 2018

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

@Zoxc
Copy link
Contributor

Zoxc commented Jan 25, 2018

not sure what the lifetime names would be? 'qx and 'gx?

Just for reference, I prefer using the same lifetime name for the global interner lifetime instead of it sometimes being 'gcx and sometimes 'tcx. So for a Qx type I'd want to use 'qx everywhere to refer to the global interner lifetime (I guess we could also consider that the query engine lifetime).

@nikomatsakis
Copy link
Contributor

I prefer using the same lifetime name for the global interner lifetime instead of it sometimes being 'gcx and sometimes 'tcx

I also think I would prefer that, though I've not tried it. Or at least I think it would be clearer.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants