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

Fri Nov 5 11:22:34 PDT 2021

Mordante marked 2 inline comments as done.
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:
> 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.
The value is stored in an exposition only `variant` in  `basic_format_arg` http://eel.is/c++draft/format.arg. (Actually it will never be stored in a `variant` since optimizations are required http://eel.is/c++draft/format#args-1. I'm working on that requirement, but nothing public yet.)

In our implementation the values are always lvalue references. P2418 allows rvalue references, but Charlie noticed some issues with the wording. So for now I prefer to keep it as is. I did some work on P2418, but I wait for an LWG-issue to see the final direction of that paper. (P2418 has been accepted at the last plenary.)

So for now I'm happy and when I implement P2418 I need to see what the exact impact rvalue references have. (I expect no changes for this test, since I haven't implemented the handle class yet.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113137