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

[Joerg Sonnenberger via Phabricator via libcxx-commits]

joerg added inline comments.


================
Comment at: libcxx/include/__config:1452
 
+#ifdef __GNUC__
+#  define _LIBCPP_PRINTFILE(fmtarg, firstvararg) \
----------------
curdeius wrote:
> Does clang-cl support this attribute? If yes, then it should be `#if defined(__GNUC_) || defined(__clang__)`.
It seems to work as is with clang-cl.


================
Comment at: libcxx/include/__config:1453
+#ifdef __GNUC__
+#  define _LIBCPP_PRINTFILE(fmtarg, firstvararg) \
+     __attribute__((__format__(__printf__, fmtarg, firstvararg)))
----------------
Quuxplusone wrote:
> 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.)
Yeah, C&P of a type. It should have been _LIBCPP_PRINTFLIKE. I'm not sure _LIBCPP_FORMAT_PRINTF is better, but I'm not really attached to the name either way.


================
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96986