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

Migrate to tail expression temporary lifetime rule in Edition 2024 #3066

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dingxiangfei2009
Copy link

@dingxiangfei2009 dingxiangfei2009 commented Aug 26, 2024

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 with valueset_arr! which must be used at the match 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.

Comment on lines 2889 to 2890
// `valueset_arr` is only suitable as a `match` scrutinee location
// because the `Value`s assume a very transient temporary lifetime
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// `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.

Copy link
Author

Choose a reason for hiding this comment

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

Applied with minor edits.

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 {
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Rename is reverted.

Copy link
Member

@hawkw hawkw left a 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.

@dingxiangfei2009 dingxiangfei2009 force-pushed the tail-expr-temp-lifetime-edition-2024 branch from 3b698b4 to 5cc96ae Compare August 30, 2024 20:48
@dingxiangfei2009
Copy link
Author

@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 match.

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