[libcxx-commits] [PATCH] D96986: [libc++] Drop template layer when using vsnprintf

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:210
+    va_start(args, msg);
+    return report_impl(ec, msg, args);
+  }
----------------
joerg wrote:
> Quuxplusone wrote:
> > It looks like you're trying to make sure that `va_end` is called even during exception-based stack unwinding, is that right? If so, I think you should do the RAII thing and create a proper struct type that calls `va_end` in its destructor. Hey, it looks like `GuardVaList` might already be that RAII type! Use it on line 209, and on line 218, and on line 113.
> > 
> > Don't call `va_end` manually on line 189 — let these `GuardVaList` objects deal with that cleanup.
> > 
> > Basically, make sure every time you call `va_start` or `va_copy`, you follow it immediately with a transfer of ownership to a `GuardVaList`.
> I don't think there is any advantage to moving GuardVaList out. If anything, calling format_string_impl first and building the result afterwards seems like the way forward as it removes the only possible exception path. 
...Actually, I take back the "RAII guard" idea. From OSX's `man stdarg`:
> Note that each call to va_start() must be matched by a call to va_end(), from within the same function.

And see https://stackoverflow.com/questions/37454179/how-to-properly-va-end

So you should just figure out a way to make `report_impl` noexcept, and then do **both** va_start and va_end from within this function. (`format_string_impl` will continue to call both va_copy and va_end from within the same function; but again, it shouldn't use the RAII type to do it; it should just be made noexcept.)

Also, please pass `va_list ap` by value, not reference; that way we match what libc does with vprintf, which we know is OK. Pass-by-reference is //probably// fine in practice, but why take the chance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96986