-
Notifications
You must be signed in to change notification settings - Fork 13k
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
rustc::ty: Rename struct_variant to non_enum_variant #47258
Conversation
src/librustc/ty/mod.rs
Outdated
@@ -1694,7 +1694,7 @@ impl<'a, 'gcx, 'tcx> AdtDef { | |||
/// Asserts this is a struct and returns the struct's unique | |||
/// variant. | |||
pub fn struct_variant(&self) -> &VariantDef { | |||
assert!(!self.is_enum()); | |||
assert!(self.is_struct()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things could change, but IIRC it was called on unions and it was supposed to be called on unions, because unions are represented as an ADT with a single "variant".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the name is misleading and the doc comment is wrong. I'm now auditing all callers to see whether they might be using this on unions. Depending on results I might change the doc comment and possibly the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non_enum_variant
is what I'd use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, introduce another method that does the same thing for unions (if union code wants the helper method but there's no code that needs to work on structs and unions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect most code using this is common for structs and unions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there's a bunch. non_enum_variant
it is.
b296578
to
38d55e6
Compare
@@ -689,7 +689,7 @@ struct AdtField<'tcx> { | |||
} | |||
|
|||
impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
fn struct_variant(&self, struct_def: &hir::VariantData) -> AdtVariant<'tcx> { | |||
fn non_enum_variant(&self, struct_def: &hir::VariantData) -> AdtVariant<'tcx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is technically a separate function that was caught up in the mass rename, but the rename fits here as well IMO.
It is also intended for use with unions.
r=me with build fixed |
38d55e6
to
cf3fefe
Compare
@bors r=eddyb |
📌 Commit cf3fefe has been approved by |
rustc::ty: Rename struct_variant to non_enum_variant r? @eddyb
r? @eddyb