axum-core/macros: Avoid double body_text() calls in rejection macros
#3110
Conversation
axum-core/src/macros.rs
Outdated
| body_text = body_text, | ||
| status = status, |
There was a problem hiding this comment.
I think these macro args are just forwarded to tracing, right? In that case, we can replace x = x with just x for args.
There was a problem hiding this comment.
yes and no. the macro-by-example pattern still expects them with the x = x syntax unfortunately, and it didn't feel worth it to add a dedicated pattern case for the shorthand syntax.
axum-core/src/macros.rs
Outdated
| @@ -49,12 +49,15 @@ macro_rules! __define_rejection { | |||
|
|
|||
| impl $crate::response::IntoResponse for $name { | |||
| fn into_response(self) -> $crate::response::Response { | |||
| let status = self.status(); | |||
| let body_text = self.body_text(); | |||
There was a problem hiding this comment.
How is using self.body_text() better than using $body? I think the latter is always a string literal. It should probably be using the literal (lit?) macro fragment specifier instead of expr.
There was a problem hiding this comment.
I think I subconsciously aimed towards having the same into_response() content for both cases, but you're right that $body is better, because body_text() turns the &'static str into a String which isn't what we want here. I've reverted that part.
There was a problem hiding this comment.
We could put the body string literal into a private associated constant for readability, I suppose. For the methods, I guess we could / should add #[inline] to fn status? It could also be a const fn. All of that isn't super important, and can be done in a separate PR though.
Maybe I'm missing something, but it looks like the
IntoResponseimplementations for the generated rejection structs are currently executing thebody_text()fn twice, which causes it to allocate memory twice. This PR extracts local variables in theinto_response()implementations to share the status and body text with the__log_rejection!()call instead.