[libcxx-commits] [PATCH] D96986: [libc++] Drop template layer when using vsnprintf
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Mar 5 16:41:50 PST 2021
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);
> 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?
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits