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

Refactor definitions of ADTs in rustc::middle::def #31010

Merged
merged 3 commits into from
Jan 21, 2016

Conversation

petrochenkov
Copy link
Contributor

All structs and their constructors are defined as DefStruct.
DefTy is splitted into DefEnum and DefTyAlias.
Ad hoc flag bool is_structure is removed from DefVariant, it was required in one place in resolve and could be obtained by other means.
Flag bool is_ctor is removed from DefFn, it wasn't really used for constructors outside of metadata decoding.

Observable effects:
More specific error messages are selected in some cases.
Two name resolution bugs fixed (#30992 and FIXME in compile-fail/empty-struct-braces-expr.rs).

Fixes #30992
Closes #30361

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -29,8 +29,9 @@ pub enum Def {
DefAssociatedConst(DefId),
DefLocal(DefId, // def id of variable
ast::NodeId), // node id of variable
DefVariant(DefId /* enum */, DefId /* variant */, bool /* is_structure */),
DefTy(DefId, bool /* is_enum */),
DefVariant(DefId /* enum */, DefId /* variant */),
Copy link
Member

Choose a reason for hiding this comment

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

Since you’re refactoring here, might it also be a good time to move from tuple-variants to struct-variants, so comments like these aren’t necessary anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and rename DefVariant to Def::Variant etc.
It would be a huge code churn and I'd like to separate changes done in this PR from pure renaming. But I can do it in this PR in separate commits if reviewer agrees.

@arielb1
Copy link
Contributor

arielb1 commented Jan 19, 2016

Renaming DefVariant to Def::Variant etc. would be fine.

Also, test needed.

r+ modulo test.

@petrochenkov
Copy link
Contributor Author

Updated with a test and renamed variants.

@nagisa
Making Def variants struct-like is too much manual work, unfortunately. I've omitted this part.

@nagisa
Copy link
Member

nagisa commented Jan 20, 2016

@bors r=arielb1 2084c2c

@arielb1
Copy link
Contributor

arielb1 commented Jan 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jan 20, 2016

📌 Commit 2084c2c has been approved by arielb1

bors added a commit that referenced this pull request Jan 21, 2016
All structs and their constructors are defined as `DefStruct`.
`DefTy` is splitted into `DefEnum` and `DefTyAlias`.
Ad hoc flag `bool is_structure` is removed from `DefVariant`, it was required in one place in resolve and could be obtained by other means.
Flag `bool is_ctor` is removed from `DefFn`, it wasn't really used for constructors outside of metadata decoding.

Observable effects:
More specific error messages are selected in some cases.
Two name resolution bugs fixed (#30992 and FIXME in compile-fail/empty-struct-braces-expr.rs).

Fixes #30992
Closes #30361
@bors
Copy link
Contributor

bors commented Jan 21, 2016

⌛ Testing commit 2084c2c with merge 51108b6...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants