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

Mark constant functions with const #1521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

julianbraha
Copy link

Using const for constant functions allows the compiler to evaluate them at compile-time, and allows the calling functions to be const, too.

These functions were detected with the clippy lint:
https://rust-lang.github.io/rust-clippy/master/index.html#/missing_const_for_fn

@julianbraha julianbraha changed the title Mark constants functions with const Mark constant functions with const Aug 30, 2024
@Drakulix
Copy link
Member

Hey, thanks for this PR, but we don't want to have every function const, just because it can be.

Said lint is allow-by-default for a reason and that is that const is a commitment to API stability, removing it later, can break existing code meaning this isn't a pure optimization. Additionally rustc should be smart enough to optimize pure accessor methods away (for those cases we rather would want to use #[inline], but that is beside the point).

However there is definitely use for const-constructors to allow said types to be used in static contexts. Similar things can be said for associated functions returning purely static information, like e.g. NodeType::minor_name_prefix.

However I would like to ask you to drop all other instances of const, if there isn't a specific reason to keep it.

@PolyMeilex
Copy link
Member

I agree here, const is used to allow usage of given function in explicit const context.
So unless we have a concrete use case for using a given API in const context, there is no reason to add it.

To illustrate the point: Constructing GbmAllocator in const context at compile time is not possible, as there is no way to acquire GbmDevice at compile time, the device has to be queried and initialized based on actual runtime hardware.

So:

  • If you want to help rustc optimize away those functions across crate boundaries, you would want to use #[inline] (I would do that for all simple public one line getters and setters)
  • If you want to execute those functions in const context at compile time, you would do const

Also, with #[inline] I would keep in mind that adding inline to functions that are rarely called would not help the performance in any way, and just increase compile times. Eg. If an object is being created only when a new GPU is plugged in, and not on every render or input frame, then optimizing its constructor is pointless.

@julianbraha
Copy link
Author

there is definitely use for const-constructors to allow said types to be used in static contexts. Similar things can be said for associated functions returning purely static information, like e.g. NodeType::minor_name_prefix.

However I would like to ask you to drop all other instances of const, if there isn't a specific reason to keep it.

I've pushed a commit to address this. I might do another pass on this, just to double-check since it's a lot of changes.

Unrelated, but my editor automatically removed trailing whitespace from the files touched by either of the two commits. Would you like me to restore the trailing whitespace?

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