Skip to content
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

Rename middle::ty::ctxt to TyCtxt #31979

Merged
merged 1 commit into from
Mar 3, 2016
Merged

Conversation

jseyfried
Copy link
Contributor

r? @eddyb

@KalitaAlexey
Copy link
Contributor

Why do we need namespaces if we are not using them?

@jseyfried
Copy link
Contributor Author

We need namespaces because we often do use them. That doesn't mean we should always avoid more descriptive names just because namespaces allow it.

In this case, I decided to use TyCtxt instead of just Ctxt to be consistent with the other contexts in the codebase (ExtCtxt, TestCtxt, CrateCtxt, InferCtxt, FnCtxt, MatchCheckCtxt, BorrowckCtxt, CheckLoanCtxt, GatherLoanCtxt, StaticInitializerCtxt, etc).

@KalitaAlexey
Copy link
Contributor

@jseyfried you can write use middle::ty::ctxt as TyCtxt in outer modules.

@jseyfried
Copy link
Contributor Author

True, but I would prefer to be consistent with the other contexts. Also, that would make it harder to grep for all uses of TyCtxt.

@eddyb
Copy link
Member

eddyb commented Mar 1, 2016

cc @nikomatsakis There is that dual type context (w/ multiple lifetimes) refactor I wanted to do ages ago, which also renamed the context, but I used TyCx IIRC (because it was shorter). Thoughts?

@KalitaAlexey
Copy link
Contributor

@jseyfried It wouldn't make harder to grep. Or I don't understood you.

@jseyfried
Copy link
Contributor Author

@KalitaAlexey You would have to grep for both ctxt and TyCtxt to find the definition and all uses.

@KalitaAlexey
Copy link
Contributor

Ok. You said all uses of TyCtxt. I separate definition from use.

@bors
Copy link
Contributor

bors commented Mar 1, 2016

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

@nikomatsakis
Copy link
Contributor

On Mon, Feb 29, 2016 at 11:48:26PM -0800, Eduard-Mihai Burtescu wrote:

cc @nikomatsakis There is that dual type context (w/ multiple lifetimes) refactor I wanted to do ages ago, which also renamed the context, but I used TyCx IIRC (because it was shorter). Thoughts?

It seems like the precedent is to call the type Ctxt and the
variables cx. e.g., InferCtxt/infcx, BorrowckCtxt/bccx,
etc. Not sure this is the best convention, but it's the one that
exists.

@eddyb
Copy link
Member

eddyb commented Mar 2, 2016

Well then, it's not like it could make refactorings any harder (actually, it should be easier to grep for TyCtxt than anything else), so LGTM.

@jseyfried jseyfried force-pushed the rename_ctxt branch 2 times, most recently from 8189937 to 5623a95 Compare March 3, 2016 06:38
@jseyfried
Copy link
Contributor Author

@eddyb rebased, the travis build is successful now

@eddyb
Copy link
Member

eddyb commented Mar 3, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2016

📌 Commit 37ba66a has been approved by eddyb

bors added a commit that referenced this pull request Mar 3, 2016
@bors
Copy link
Contributor

bors commented Mar 3, 2016

⌛ Testing commit 37ba66a with merge f6e125f...

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.

5 participants