feat: Allow fmt::join to accept rvalue tuples#4554
Closed
teruyamato0731 wants to merge 3 commits intofmtlib:masterfrom
Closed
feat: Allow fmt::join to accept rvalue tuples#4554teruyamato0731 wants to merge 3 commits intofmtlib:masterfrom
teruyamato0731 wants to merge 3 commits intofmtlib:masterfrom
Conversation
9027f7b to
4e2db10
Compare
Contributor
|
Thanks for the PR but this handles only one case of dangling references and introduces a major inconsistency in handling rvalue and lvalue references. Therefore I don't think we should go this route and instead we should try to prevent dangling references for both rvalue and lvalue references in a similar way. b5b9317 implements the first step in this direction by adding a warning on unsafe use for clang 12+: auto format_as(const SocketAddr& s) {
// std::tie returns a temporary (rvalue) tuple of references.
return fmt::join(std::tie(s.addr, s.port), ":");
}https://www.godbolt.org/z/4zTnWcKvc We could extend this to other compilers and turn this warning into an error by default. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates
fmt::joinand its underlyingtuple_join_viewto accept rvalue tuples. This enhances usability and fixes a critical lifetime issue, particularly when usingfmt::joinwithinformat_asspecializations.The Problem
Currently,
fmt::joinonly accepts an lvalue reference to a tuple. This prevents passing a temporary tuple, such as one returned fromstd::make_tupleorstd::tie, directly to the function.This limitation is particularly dangerous when implementing a
format_asoverload. For example:In the code above, the
tuple_join_viewreturned byfmt::joinholds a dangling reference to the temporary tuple created bystd::tie, which is destroyed at the end of theformat_asfunction. This leads to undefined behavior when the view is later used for formatting.The Solution
This PR resolves the issue by changing
fmt::jointo accept tuples via a forwarding reference (T&&).tuple_join_view, extending its lifetime and preventing dangling references.With this change, the
format_asimplementation shown above becomes safe and works as intuitively expected. This makesfmt::joina much more powerful and safe tool for creating custom formatters for aggregate types.