-
Notifications
You must be signed in to change notification settings - Fork 722
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
Migrate to tail expression temporary lifetime rule in Edition 2024 #3066
base: master
Are you sure you want to change the base?
Migrate to tail expression temporary lifetime rule in Edition 2024 #3066
Conversation
0543b30
to
ad1990b
Compare
tracing/src/macros.rs
Outdated
// `valueset_arr` is only suitable as a `match` scrutinee location | ||
// because the `Value`s assume a very transient temporary lifetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// `valueset_arr` is only suitable as a `match` scrutinee location | |
// because the `Value`s assume a very transient temporary lifetime | |
// `valueset_arr` is need as a `match` scrutinee location. `Value`s | |
// assume a transient, temporary lifetime. this is necessary to support | |
// https://github.com/rust-lang/rfcs/pull/3606. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied with minor edits.
tracing/src/macros.rs
Outdated
macro_rules! valueset { | ||
// `valueset_arr` is only suitable as a `match` scrutinee location | ||
// because the `Value`s assume a very transient temporary lifetime | ||
macro_rules! valueset_arr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the rename? I would maybe consider keeping it the same name, especially since this is a hidden, semver-exempt API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Rename is reverted.
ad1990b
to
3b698b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided that all the tests pass with this change, this seems good to me --- thank you. It might be worth commenting on the individual instances of the match
to refer to the comment explaining why they're necessary, but it's not a big deal.
3b698b4
to
5cc96ae
Compare
@hawkw Thanks for the suggestion. I feel that leaving comments in the macro body would make them stay in the macro expansion, so I took the liberty to lift the explanation to the top of the macro definition. Hopefully this is clear enough to explain the use of |
Motivation
Related to rust-lang/rust#123739
This is a migration effort to prepare
tracing
macros to be more Edition 2024 oriented.This change is backward-compatible so that the macro works exactly the same in terms of program semantics in both Edition 2018 and prospective Edition 2024.
cc @jieyouxu @traviscross
Solution
valueset!
is replaced withvalueset_arr!
which must be used at thematch
scrutinee location.__tracing_log!
macro is written to take a "continuation" so that the&'_ ValueSet
is generated and passed into the expanded code only when the logger is enabled.