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

refactor: make status a const function in rejection handling #3168

Merged
merged 2 commits into from
Jan 11, 2025

Conversation

TheAwiteb
Copy link
Contributor

@TheAwiteb TheAwiteb commented Jan 10, 2025

Fix #3167, Updated the status method to be a const function for improved performance and compile-time evaluation.

Motivation

Allows the status code to be evaluated at compile time

Solution

-pub fn status(&self) -> http::StatusCode {
+pub const fn status(&self) -> http::StatusCode {

Copy link
Collaborator

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

could you add the const keyword for the second macro variant too?

and do you know if we could do the same for __composite_rejection?

@TheAwiteb
Copy link
Contributor Author

could you add the const keyword for the second macro variant too?

No, because it uses ToString::to_string, which is not a const function.

and do you know if we could do the same for __composite_rejection?

Yes I can, for the status function only

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 10, 2025

No, because it uses ToString::to_string, which is not a const function.

I'm talking about the function in line 115, and I don't see a to_string() call in there :)

- Updated the `status` method to be a const function for improved
performance and compile-time evaluation.

Signed-off-by: Awiteb <a@4rs.nl>
@TheAwiteb TheAwiteb force-pushed the make-status-function-const branch from 8d86a54 to 101d446 Compare January 10, 2025 18:35
@TheAwiteb
Copy link
Contributor Author

FailedToDeserializePathParams and InvalidUtf8InPathParam are used in the __composite_rejection macro, but their status functions are not implemented by the __define_rejection macro. Therefore, I made their status functions const.

@TheAwiteb TheAwiteb requested a review from Turbo87 January 10, 2025 18:40
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Makes sense to me, but please note that this does not impact performance in any way. const fn means it can be evaluated in const contexts such as const ITEMs and statics, it has no influence on whether optimizations like constant folding are applied in code outside of a const context.

@jplatte
Copy link
Member

jplatte commented Jan 11, 2025

Could you add changelog entries to axum and axum-extra (where the macro-generated methods live)?

Signed-off-by: Awiteb <a@4rs.nl>
@TheAwiteb TheAwiteb requested a review from jplatte January 11, 2025 18:47
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks!

@jplatte jplatte enabled auto-merge (squash) January 11, 2025 18:48
@jplatte jplatte merged commit e09cc59 into tokio-rs:main Jan 11, 2025
18 checks passed
@TheAwiteb TheAwiteb deleted the make-status-function-const branch January 11, 2025 18:52
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.

Make status function that generated by __define_rejection a const function
3 participants