[libcxx-commits] [PATCH] D113137: [libc++] Fix lifetime issues of temporaries.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp:54
+  auto format_arg_store = std::make_format_args<std::basic_format_context<OutIt, CharT>>(true, CharT('a'), 42, string);
+  std::basic_format_args args{format_arg_store};
 
----------------
Quuxplusone wrote:
> Style: `std::basic_format_args args = format_arg_store;` (it's not an aggregate or sequence-container)
> 
> Do I understand correctly that `std::make_format_args` does not actually return a `std::format_args`? Who came up with that? 😛 
> 
> But does this fix the dangling? https://en.cppreference.com/w/cpp/utility/format/make_format_args insists that "A formatting argument has reference semantics for user-defined types and does not extend the lifetime of `args`," so do you really need to do something like
> ```
> auto args_tuple = std::make_tuple(true, CharT('a'), 42, string);
> auto args = std::apply([](auto&&... ts) {
>     return std::make_format_args<std::basic_format_context<OutIt, CharT>>(static_cast<decltype(ts)>(ts)...);
> }, std::tuple<bool&&, CharT&&, int&&, std::basic_string<CharT>&>(args_tuple));
> ```
> ? (Or consider turning each of the following blocks into its own function, and just calling them like `test_foo(std::make_format_args<...>(...));` which would eliminate the lifetime issues in a more realistic way: basically by treating `format_args` as a parameter-only type.)
> Style: `std::basic_format_args args = format_arg_store;` (it's not an aggregate or sequence-container)
> 
> Do I understand correctly that `std::make_format_args` does not actually return a `std::format_args`? Who came up with that? 😛 

Indeed
```
Returns an object that stores an array of formatting arguments and can be implicitly converted to std::basic_format_args<Context>. 
```
The `basic_format_args` stores a pointer to the data of this array. Before the change that array was a temporary, after the change it's no longer a temporary. (The format functions are implemented like `return vformat_to(out, fmt.str, make_format_args(args...));`. So the temporary's lifetime isn't an issue there.)

> But does this fix the dangling? https://en.cppreference.com/w/cpp/utility/format/make_format_args insists that "A formatting argument has reference semantics for user-defined types and does not extend the lifetime of `args`," so do you really need to do something like
> ```
> auto args_tuple = std::make_tuple(true, CharT('a'), 42, string);
> auto args = std::apply([](auto&&... ts) {
>     return std::make_format_args<std::basic_format_context<OutIt, CharT>>(static_cast<decltype(ts)>(ts)...);
> }, std::tuple<bool&&, CharT&&, int&&, std::basic_string<CharT>&>(args_tuple));
> ```
> ? (Or consider turning each of the following blocks into its own function, and just calling them like `test_foo(std::make_format_args<...>(...));` which would eliminate the lifetime issues in a more realistic way: basically by treating `format_args` as a parameter-only type.)

No this should work; the values of the fundamental types are copied in the array so they won't dangle. The emphasis in the note is `user-defined types`. Using a temporary would be an issue for `std::string`, it's stored as a `std::string_view` in the array. That's why the argument `string` isn't a temporary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113137/new/

https://reviews.llvm.org/D113137