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

Add const to constructors by a feature #218

Merged
merged 17 commits into from
Jan 31, 2023

Conversation

fmiguelgarcia
Copy link
Contributor

@fmiguelgarcia fmiguelgarcia commented Nov 10, 2022

Resolves Constructor generated new should be marked const

Synopsis

Added a new const feature which generates const fn on Constructor derives.

Solution

Checklist

  • Documentation is updated
  • Tests are added/updated IMPORTANT: I am not sure if new tests are added to the CI.
  • [CHANGELOG entry][l:1] is added

@tyranron
Copy link
Collaborator

Should this really be feature-gated? I cannot imagine the situation where const will be harmful on constructors.

@fmiguelgarcia
Copy link
Contributor Author

Should this really be feature-gated? I cannot imagine the situation where const will be harmful on constructors.

Just in case you are using a legacy compiler with no support of const in Generics.
Having said that, I think const feature should be enable by default, WDYT?

att. @tyranron

@tyranron
Copy link
Collaborator

@fmiguelgarcia well, once we release 1.0 (next release) we will have MSRV set to 1.65, so you won't be able to use legacy compilers 🤷‍♂️

@JelteF
Copy link
Owner

JelteF commented Nov 14, 2022

Yes, given we're going dropping support for old rust versions in the next release I think this shouldn't be behind a feature, but should simply be the only behaviour.

@JelteF
Copy link
Owner

JelteF commented Nov 14, 2022

Also, I feel like the same should be done for the IsVariant and Unwrap derives.

@tyranron
Copy link
Collaborator

tyranron commented Nov 14, 2022

@JelteF IsVariant yes, but Unwrap definitely not, as const at the moment is not very clever with destructor evaluation checks, so almost any struct/enum destruction in const leads to compilation error. See rust-lang/rust#60964 and rust-lang/rust#73255 for details.

@fmiguelgarcia
Copy link
Contributor Author

@tyranron I've used the const feature to enable const in Unwrap (I've just read your comment).
IsVariant and Constructor now uses const fn.

Do you want to remove const feature completely? Or Do you want to keep it and support const in Unwrap?
att. @JelteF @tyranron

@tyranron
Copy link
Collaborator

@fmiguelgarcia I'd vote for removing const feature completely. We always may constify Unwrap once feature(const_precise_live_drops) is stabilized. It's not something that vital, deserving to squat-around it with an additional Cargo feature.

@@ -24,12 +24,14 @@ pub fn expand(input: &DeriveInput, _: &str) -> TokenStream {
_ => panic!("Only structs can derive a constructor"),
};
let original_types = &get_field_types(&fields);
let fn_signature = quote!( pub const fn new(#(#vars: #original_types),*) -> #input_type #ty_generics );
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't have to be a separate variable, I'm guessing this is a leftover from the feature.

@JelteF JelteF added this to the 1.0.0 milestone Nov 15, 2022
@JelteF
Copy link
Owner

JelteF commented Nov 15, 2022

Something is weird about the github diff, I'm not sure why but it shows changes introduced in other PRs. You might want to try squashing your commits together and rebasing on top of master.

@JelteF
Copy link
Owner

JelteF commented Jan 24, 2023

I think this is almost good to merge, just a few things are needed:

  1. Remove const from unwrap due to the bugs (so only const for constructor and isvariant for now)
  2. Remove the const feature and simply always use const.
  3. fix clippy errors.

@JelteF
Copy link
Owner

JelteF commented Jan 24, 2023

@fmiguelgarcia do you have time to do that? This is one of the final things left on the 1.0.0 milestone.

@tyranron tyranron requested a review from JelteF January 30, 2023 17:41
@tyranron tyranron merged commit 273b947 into JelteF:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constructor generated new should be marked const
3 participants