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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__config:1453
+#ifdef __GNUC__
+#  define _LIBCPP_PRINTFILE(fmtarg, firstvararg) \
+     __attribute__((__format__(__printf__, fmtarg, firstvararg)))
----------------
curdeius wrote:
> The macro name is misleading for me. It doesn't print, and it doesn't print files. It annotates a printf-like function though.
> IMO it should contain something like format and attribute in the name, but I haven't given it a long thought.
Looks like the typical name would be `_LIBCPP_FORMAT_PRINTF`. (Compare `_LIBCPP_NOALIAS`, `_LIBCPP_NORETURN`, `_LIBCPP_ALWAYS_INLINE`, etc.)


================
Comment at: libcxx/src/filesystem/filesystem_common.h:210
+    va_start(args, msg);
+    return report_impl(ec, msg, args);
+  }
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96986