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 on some functions #22

Merged
merged 1 commit into from
Mar 11, 2023
Merged

Conversation

jaudiger
Copy link

@jaudiger jaudiger commented Mar 6, 2023

Constify some functions, in order to use them in const context for other crates.

@jaudiger jaudiger changed the title a const on some functions Add const on some functions Mar 6, 2023
@nickelc
Copy link

nickelc commented Mar 7, 2023

If you move the Style::default() details into a private Style::const_default method then you can also constify Style::new() and Style::is_plain().

@sholderbach
Copy link
Member

I know for nushell it doesn't matter as much as we are pretty aggressive in that regard but do we want to commit to a certain minimum supported Rust version? Then I am not fully up to speed how far we can go with constification without precluding certain users.

@jaudiger
Copy link
Author

jaudiger commented Mar 7, 2023

I can revert some const functions if needed. For example, to not enforce a specific rust version, I can remove the const keywords when the saturating functions are involved, and so on. What your thoughts on that?

I can suggest two approaches for now:

  • imply const on easy functions that don’t rely on sub const functions from rust
  • or just close this PR

@jaudiger
Copy link
Author

jaudiger commented Mar 7, 2023

At a certain point, my initial assumption was: most of the API exposed by this crate could be const. Since there is no a strong logic in there. And I was thinking that enforcing this trait could make sense for the end users. Anyway, you’re right about the no usage of a rust version as of today.

Just let me know, what would you prefer.

@nickelc
Copy link

nickelc commented Mar 7, 2023

I've tested the PR with cargo msrv and it bumps the MSRV from 1.56 to 1.61.

@sholderbach
Copy link
Member

I've tested the PR with cargo msrv and it bumps the MSRV from 1.56 to 1.61.

Thanks for investigating that!

For the stuff I am doing both would be fine.

@jaudiger
Copy link
Author

jaudiger commented Mar 8, 2023

So, what's next @sholderbach? Should I enforce the usage of the Rust version in cargo.toml without doing any further modifications?

About your comment @nickelc, bevy did already that in their latest release (https://bevyengine.org/news/bevy-0-10/#const-bevy-ui-defaults). It could enable the usage of const in additional functions until Rust enables it inside the Default trait.

@jaudiger
Copy link
Author

jaudiger commented Mar 10, 2023

I just rebased this PR on main, and I added the field rust-version inside Cargo.toml.

@jaudiger
Copy link
Author

@fdncred I don't know what is the current strategy regarding const functions. At least, the MSRV now announced is 1.62.1 with #30. There is no more blocker regarding adding those const keywords.

But I'm open to any discussion. I could reduce the set of APIs using const, or we can even close it if this is not the right time !

@sholderbach
Copy link
Member

As we already bumped the MSRV for that default syntactic sugar we can go ahead with this constification.

@sholderbach sholderbach merged commit dcb7273 into nushell:main Mar 11, 2023
@jaudiger jaudiger deleted the const_functions branch May 17, 2024 15:34
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.

3 participants