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

Modernize type names in the compiler #26895

Merged
merged 4 commits into from
Jul 12, 2015

Conversation

jroesch
Copy link
Member

@jroesch jroesch commented Jul 8, 2015

This PR modernizes some names in the type checker. The only remaining snake_case name in ty.rs is ctxt which should be resolved by @eddyb's pending refactor. We can bike shed over the names, it would just be nice to bring the type checker inline with modern Rust.

r? @eddyb

cc @nikomatsakis

@jroesch jroesch force-pushed the modernize-typeck-names branch from e62e5f8 to 754aaea Compare July 8, 2015 19:38
@arielb1
Copy link
Contributor

arielb1 commented Jul 8, 2015

TypeWithMutability seems very verbose. It is basically usefully used about three times (as the output of ty::deref, in Ty, and uselessly in Field) - I would rather remove it.

@jroesch
Copy link
Member Author

jroesch commented Jul 8, 2015

@arielb1 totally okay with bike shedding or removal. I'll remove it and tack another commit on here.

@@ -278,7 +278,7 @@ impl<'a, 'tcx> CombineFields<'a, 'tcx> {
};
let u = ty.fold_with(&mut generalize);
if generalize.cycle_detected {
Err(ty::terr_cyclic_ty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate to ask for this, but I'd rather have ty::TypeError::CyclicTy (i.e., not pub use all the names from the enum)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wanted to do it, but wasn't sure how much pain it was to fix. I'll bite the bullet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you be okay with just TypeError::Variant? I think it is slightly less verbose and just as clear.

@nikomatsakis
Copy link
Contributor

lgtm

@nikomatsakis
Copy link
Contributor

r=me whenever you feel you've suffered enough :)

@bkoropoff
Copy link
Contributor

+1, this always tripped me up.

@jroesch
Copy link
Member Author

jroesch commented Jul 12, 2015

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 12, 2015

📌 Commit 19218ee has been approved by nikomatsakis

bors added a commit that referenced this pull request Jul 12, 2015
This PR modernizes some names in the type checker. The only remaining snake_case name in ty.rs is `ctxt` which should be resolved by @eddyb's pending refactor. We can bike shed over the names, it would just be nice to bring the type checker inline with modern Rust.

r? @eddyb 

cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Jul 12, 2015

⌛ Testing commit 19218ee with merge adcae00...

@bors bors merged commit 19218ee into rust-lang:master Jul 12, 2015
@jroesch jroesch deleted the modernize-typeck-names branch July 12, 2015 22:11
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.

6 participants