[libcxx-commits] [PATCH] D98097: [libc++] "Merged wording" for D98077 and D96986

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.





================
Comment at: libcxx/include/__config:1462
+#if defined(__GNUC__) || defined(__clang__)
+#define _LIBCPP_FORMAT_PRINTF(...)                                             \
+  __attribute__((__format__(__printf__, __VA_ARGS__)))
----------------
Wouldn't it be better use 2 named arguments instead of `...`. The latter isn't officially part of C++98 and the attribute requires 3 arguments.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:81
+    ret = ::vsnprintf(&result[0], size_with_null, msg, ap);
+    _LIBCPP_ASSERT(static_cast<size_t>(ret) == (size_with_null - 1), "TODO");
   }
----------------
Not too fond of the number of `static_cast`s and the `"TODO"`, but it seems they were already there.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:88
+format_string(const char* msg, ...) {
+  std::string ret;
+  va_list ap;
----------------
For consistency can you remove `std::`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98097