-
Notifications
You must be signed in to change notification settings - Fork 896
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
Add dedicated structs for BindingKind
variants
#3672
Conversation
pub name: &'a str, | ||
/// The full name of the submodule being imported. | ||
/// Given `import foo.bar`, `full_name` would be "foo.bar". | ||
pub full_name: &'a str, |
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 think we can improve some of these abstractions (better names, shared traits). This PR doesn't attempt to improve the object hierarchy at all -- it's just a "straight" migration.
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.
Sounds good. I hoped to find a name in the python specification but didn't find any other than dotted name. What came to my mind is that this is the path but there's no precedence for this term
StarImportation(StarImportation), | ||
Importation(Importation<'a>), | ||
FromImportation(FromImportation<'a>), | ||
SubmoduleImportation(SubmoduleImportation<'a>), |
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.
Learning opportunity for me: when would we want to Box
these?
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.
When we want the enum to have the size of a box instead of the biggest struct, I think.
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.
Boxing a variant reduces the size of that variant to a single pointer instead of the size of the inner struct + padding.
I recommend boxing when one variant is significantly larger than all other variants.
edcdf3f
to
8759785
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
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.
🎸🎸
BindingKind::Importation(.., full_name) => full_name, | ||
BindingKind::FromImportation(.., full_name) => full_name.as_str(), | ||
BindingKind::SubmoduleImportation(.., full_name) => full_name, | ||
BindingKind::Importation(Importation { full_name, .. }) => full_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.
Nit. This could be moved to a 'full_name' method on BindingKind to avoid repetition
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.
It doesn't exist on all variants though -- so would it return Option<&str>
?
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, it would have to return an Option
@@ -366,6 +385,54 @@ impl nohash_hasher::IsEnabled for BindingId {} | |||
// StarImportation | |||
// FutureImportation | |||
|
|||
#[derive(Clone, Debug)] | |||
pub struct Export { | |||
/// The names of the bindings exported via `__all__`. |
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.
Thanks for commenting the fields.
pub name: &'a str, | ||
/// The full name of the submodule being imported. | ||
/// Given `import foo.bar`, `full_name` would be "foo.bar". | ||
pub full_name: &'a str, |
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.
Sounds good. I hoped to find a name in the python specification but didn't find any other than dotted name. What came to my mind is that this is the path but there's no precedence for this term
StarImportation(StarImportation), | ||
Importation(Importation<'a>), | ||
FromImportation(FromImportation<'a>), | ||
SubmoduleImportation(SubmoduleImportation<'a>), |
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.
Boxing a variant reduces the size of that variant to a single pointer instead of the size of the inner struct + padding.
I recommend boxing when one variant is significantly larger than all other variants.
Summary
This is a non-behavior-changing refactor to move the
BindingKind
variants to use dedicated structs.Test Plan
Verify that all automated checks pass as expected.