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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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};
 
----------------
Mordante wrote:
> 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.
> 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.

Okay, everything you say is true for libc++'s implementation — we copy `int`s but merely reference/view `std::string`s — but does the Standard actually mandate that that happens, and if so, how? (Does it actually mandate value semantics for a whitelist of primitive types, reference semantics for "everything else"? Is there any gray area? e.g. what happens for `int128_t`, `std::byte`, non-const-`void*`, `void(*)()`?)  Is it //possible// that a "maliciously conforming" implementation might capture your `int` argument by reference, even if libc++ never will? We do want the test suite to work even for non-libc++ implementations... although I think it's quite defensible in this case to say "we'll cross that bridge if we ever come to it."

(My vague unease with this is probably refactoring-related. What if somebody six months from now notices that we test `make_format_args` with a `string` lvalue but never with a `string` rvalue: will it be easy for them to add a test for `string` rvalues, or will they have to reinvent all the stuff we're going through now?)

Anyway, I'm not blocking this; I still say if you're happy with it (and Louis isn't //un//happy with it) then it should be shipped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113137