Remove double forward #1387
Merged
Remove double forward #1387
Conversation
This file contains 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
dmachaj
reviewed
Jan 17, 2024
| WINRT_ASSERT(size < UINT_MAX); | ||
| auto const size32 = static_cast<uint32_t>(size); | ||
|
|
||
| hstring_builder builder(size32); | ||
| WINRT_VERIFY_(size32, std::format_to_n(builder.data(), size32, std::forward<Args>(args)...).size); | ||
| WINRT_VERIFY_(size32, std::format_to_n(builder.data(), size32, args...).size); |
There was a problem hiding this comment.
It seems like you could still forward here. Although I guess without the first forward there's not much point anymore...?
There was a problem hiding this comment.
While formatters don't care about value category, apparently std::format_string does. So forwarding in one place but not the other would break it (like it currently is).
|
Looks like this change introduced some build breaks in the cpp20 test code :( |
|
Yeah, should've actually tested it haha. Lemme fix that. |
|
Should work now :) |
dmachaj
approved these changes
Jan 22, 2024
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.
This fixes the code analysis warnings pointed out by @dmachaj. We can't just really switch to using
Args const&...because some types cannot be formatted as const lvalues - most notablyranges::filter_view. So instead, we take a forwarding reference, but don't actually callstd::forward, resulting in all the arguments being passed tostd::formatas lvalue references.Formatters that care about the exact value category should feel bad.