[libc++] "Merged wording" for D98077 and D96986

[• Quuxplusone]

Summary

This is my attempt to merge D98077 (bugfix the format strings for Windows paths, which use wchar_t not char) and D96986 (replace C++ variadic templates with C-style varargs so that __attribute__((format(printf))) can be applied, for better safety). Those two patches merge-conflict with each other, and also I had some pretty heavy "requested changes" on D96986, so I decided to just show what I mean directly.

The one intentional functional change here is in __create_what. It now prints path1 and path2 in square-brackets and double-quotes, rather than just square-brackets. Prior to this patch, it would print either path double-quoted if-and-only-if it was the empty string. Now the double-quotes are always present. I doubt anybody's code is relying on the current format, right?

(Added @EricWF as the original author of this code, in 9158bfd32ebd4)

[• Quuxplusone]

And of course as soon as I look at the diff I spot a missing va_end. :) Hopefully fixed now.

[joerg]

That covers my case. I'm not completely happy with the need to rethrow, but the alternatives are much worse. It's a bit sad that we can't optimize it away, too.

[• Quuxplusone]

Adjust expected output in tests.
Guard _LIBCPP_FORMAT_PRINTF with defined(__GNUC__) || defined(__clang__) so as to pick up clang-cl support.

[curdeius]

Apart from the fact that it fails with exceptions disabled, I have no other remarks. Generally it's ok.
I'll have another look at it when it gets green.

[• Quuxplusone]

Guard try/catch for the no-exceptions build.
Fix a few things clang-format was complaining about. But I'm not making clang-format totally happy, and I think that's okay because some of what it's asking for is dumb.

[curdeius]

LGTM pending CI.

[mstorsjo]

LGTM from my point of view, but this still needs another approver (or can @Quuxplusone self-approve for the second approval)? Or can @Mordante or @zoecarver have a look?

[Mordante]

libcxx/include/__config

1462

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.

libcxx/src/filesystem/filesystem_common.h

71

Not too fond of the number of static_casts and the "TODO", but it seems they were already there.

88

For consistency can you remove std::?

[• Quuxplusone]

libcxx/include/__config

1462

I had thought I was copying style from elsewhere in this file, but I guess only _LIBCPP_DIAGNOSE_WARNING/_LIBCPP_DIAGNOSE_ERROR actually use ... for this purpose. So the only benefit I'm getting from ... is that I don't have to worry about naming or whitespace conventions.
How do we feel about

#define _LIBCPP_FORMAT_PRINTF(a, b)   \
  __attribute__((__format__(__printf__, a, b)))

We already use a, b as macro parameter names on line 227. If this naming/whitespace looks acceptable, I don't mind making this change.

FWIW, I claim C++98-portability is not a concern, given that we already use ... specifically in C++98 mode, e.g. in #define static_assert(...) on line 898.

[joerg]

libcxx/include/__config

1462

Macro arguments are limited to the expansion as scope and disappear afterwards. They don't interact with tokens outside the macro expansion. I don't think compatibility with C++98 is a concern here, but there is no reason to give up the checking on the number of arguments here. E.g. in NetBSD they are called fmtarg and firstvararg to make it self-documenting.

[Mordante]

libcxx/include/__config

1462

You're right we already use ... in C++98 mode. I prefer named arguments if the number required is fixed. I like the ones @joerg proposes. I had to read the manual to see what the syntax of the attribute was.

[• Quuxplusone]

libcxx/include/__config

1462

I strongly prefer not fmtarg, firstvararg, but I'll do a, b.
(Note that as with any attribute that names parameters by index, there are issues such as whether to count this and whether to index starting at 0 or 1. I also would have to look up the exact semantics, or better, cut-and-paste from somewhere that already got it right.)

[• Quuxplusone]

Addressed @Mordante's comments. Ping for approval!

[• Quuxplusone]

libcxx/src/filesystem/filesystem_common.h

88

Done (and also on line 71). This file does contain some std::s, on declval and array, but not on string. (Personally I wish it would put std:: on everything, just like user code should. We're not doing anything "special" in this file that requires us to deviate from the usual good practice; the current use of unqualified string is just inessential mental noise IMO.)

[mstorsjo]

Also regarding this one, before landing it, it'd be good with a more descriptive subject - this one makes sense during review, but makes less sense when looking back at the lone commit afterwards.

[• Quuxplusone]

@mstorsjo: I'm planning to include the revision summary ("This is my attempt to merge ... format, right?") in the commit message. I'm failing to come up with a good title line. Maybe [libc++] Apply attribute(format(printf)) and Win32 fixes to filesystem helpers?

[mstorsjo]

In D98097#2624245, @Quuxplusone wrote:

@mstorsjo: I'm planning to include the revision summary ("This is my attempt to merge ... format, right?") in the commit message. I'm failing to come up with a good title line. Maybe [libc++] Apply attribute(format(printf)) and Win32 fixes to filesystem helpers?

I guess that's as concise as it gets, yeah - that sounds good to me.

[Mordante]

LGTM!

libcxx/src/filesystem/filesystem_common.h

88

I don't mind adding std::. Just as long as the code is consistent.

libcxx/include/__config

 #endif
 
 #if __has_attribute(init_priority)
 # define _LIBCPP_INIT_PRIORITY_MAX __attribute__((init_priority(101)))
 #else
 # define _LIBCPP_INIT_PRIORITY_MAX
 #endif
 
+#if defined(__GNUC__) || defined(__clang__)
+#define _LIBCPP_FORMAT_PRINTF(a, b)                                            \
+  __attribute__((__format__(__printf__, a, b)))
+#else
+#define _LIBCPP_FORMAT_PRINTF(a, b)
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/src/filesystem/directory_iterator.cpp

directory_iterator& directory_iterator::__increment(error_code* ec) {
   _LIBCPP_ASSERT(__imp_, "Attempting to increment an invalid iterator");
   ErrorHandler<void> err("directory_iterator::operator++()", ec);
 
   error_code m_ec;
   if (!__imp_->advance(m_ec)) {
     path root = move(__imp_->__root_);
     __imp_.reset();
     if (m_ec)
-      err.report(m_ec, "at root \"%s\"", root);
+      err.report(m_ec, "at root " PATH_CSTR_FMT, root.c_str());
   }
   return *this;
 }
 
 directory_entry const& directory_iterator::__dereference() const {
   _LIBCPP_ASSERT(__imp_, "Attempting to dereference an invalid iterator");
   return __imp_->__entry_;
 }
while (stack.size() > 0) {
     if (m_ec)
       break;
     stack.pop();
   }
 
   if (m_ec) {
     path root = move(stack.top().__root_);
     __imp_.reset();
-    err.report(m_ec, "at root \"%s\"", root);
+    err.report(m_ec, "at root " PATH_CSTR_FMT, root.c_str());
   } else {
     __imp_.reset();
   }
 }
 
 bool recursive_directory_iterator::__try_recursion(error_code* ec) {
   ErrorHandler<void> err("recursive_directory_iterator::operator++()", ec);
 
if (m_ec) {
     const bool allow_eacess =
         bool(__imp_->__options_ & directory_options::skip_permission_denied);
     if (m_ec.value() == EACCES && allow_eacess) {
       if (ec)
         ec->clear();
     } else {
       path at_ent = move(curr_it.__entry_.__p_);
       __imp_.reset();
-      err.report(m_ec, "attempting recursion into \"%s\"", at_ent);
+      err.report(m_ec, "attempting recursion into " PATH_CSTR_FMT,
+                 at_ent.c_str());
     }
   }
   return false;
 }
 
 _LIBCPP_END_NAMESPACE_FILESYSTEM

libcxx/src/filesystem/filesystem_common.h

 
 #if defined(__GNUC__) || defined(__clang__)
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-function"
 #endif
 
 #if defined(_LIBCPP_WIN32API)
 #define PS(x) (L##x)
+#define PATH_CSTR_FMT "\"%ls\""
 #else
 #define PS(x) (x)
+#define PATH_CSTR_FMT "\"%s\""
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 namespace detail {
 
 #if defined(_LIBCPP_WIN32API)
 // Non anonymous, to allow access from two translation units.
 errc __win_err_to_errc(int err);
 #endif
 
 namespace {
 
-static string format_string_imp(const char* msg, ...) {
-  // we might need a second shot at this, so pre-emptivly make a copy
-  struct GuardVAList {
-    va_list& target;
-    bool active = true;
-    GuardVAList(va_list& tgt) : target(tgt), active(true) {}
-    void clear() {
-      if (active)
-        va_end(target);
-      active = false;
-    }
-    ~GuardVAList() {
-      if (active)
-        va_end(target);
-    }
-  };
-  va_list args;
-  va_start(args, msg);
-  GuardVAList args_guard(args);
-
-  va_list args_cp;
-  va_copy(args_cp, args);
-  GuardVAList args_copy_guard(args_cp);
-
-  std::string result;
-
-  array<char, 256> local_buff;
-  size_t size_with_null = local_buff.size();
-  auto ret = ::vsnprintf(local_buff.data(), size_with_null, msg, args_cp);
-
-  args_copy_guard.clear();
-
-  // handle empty expansion
-  if (ret == 0)
-    return result;
-  if (static_cast<size_t>(ret) < size_with_null) {
-    result.assign(local_buff.data(), static_cast<size_t>(ret));
-    return result;
-  }
-
+static _LIBCPP_FORMAT_PRINTF(1, 0) string
+format_string_impl(const char* msg, va_list ap) {
+  array<char, 256> buf;
+
+  va_list apcopy;
+  va_copy(apcopy, ap);
+  int ret = ::vsnprintf(buf.data(), buf.size(), msg, apcopy);
+  va_end(apcopy);
+
+  string result;
+  if (static_cast<size_t>(ret) < buf.size()) {
+    result.assign(buf.data(), static_cast<size_t>(ret));
+  } else {
     // we did not provide a long enough buffer on our first attempt. The
     // return value is the number of bytes (excluding the null byte) that are
     // needed for formatting.
-  size_with_null = static_cast<size_t>(ret) + 1;
+    size_t size_with_null = static_cast<size_t>(ret) + 1;
     result.__resize_default_init(size_with_null - 1);
-  ret = ::vsnprintf(&result[0], size_with_null, msg, args);
+    ret = ::vsnprintf(&result[0], size_with_null, msg, ap);
     _LIBCPP_ASSERT(static_cast<size_t>(ret) == (size_with_null - 1), "TODO");
-
-  return result;
   }
-
-const path::value_type* unwrap(path::string_type const& s) { return s.c_str(); }
-const path::value_type* unwrap(path const& p) { return p.native().c_str(); }
-template <class Arg>
-Arg const& unwrap(Arg const& a) {
-  static_assert(!is_class<Arg>::value, "cannot pass class here");
-  return a;
+  return result;
 }
 
-template <class... Args>
-string format_string(const char* fmt, Args const&... args) {
-  return format_string_imp(fmt, unwrap(args)...);
+static _LIBCPP_FORMAT_PRINTF(1, 2) string
+format_string(const char* msg, ...) {
+  string ret;
+  va_list ap;
+  va_start(ap, msg);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  try {
+#endif  // _LIBCPP_NO_EXCEPTIONS
+    ret = format_string_impl(msg, ap);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  } catch (...) {
+    va_end(ap);
+    throw;
+  }
+#endif  // _LIBCPP_NO_EXCEPTIONS
+  va_end(ap);
+  return ret;
 }
 
 error_code capture_errno() {
   _LIBCPP_ASSERT(errno, "Expected errno to be non-zero");
   return error_code(errno, generic_category());
 }
 
 #if defined(_LIBCPP_WIN32API)
T report(const error_code& ec) const {
     case 1:
       __throw_filesystem_error(what, *p1_, ec);
     case 2:
       __throw_filesystem_error(what, *p1_, *p2_, ec);
     }
     _LIBCPP_UNREACHABLE();
   }
 
-  template <class... Args>
-  T report(const error_code& ec, const char* msg, Args const&... args) const {
+  _LIBCPP_FORMAT_PRINTF(3, 0)
+  void report_impl(const error_code& ec, const char* msg, va_list ap) const {
     if (ec_) {
       *ec_ = ec;
-      return error_value<T>();
+      return;
     }
     string what =
-        string("in ") + func_name_ + ": " + format_string(msg, args...);
+        string("in ") + func_name_ + ": " + format_string_impl(msg, ap);
     switch (bool(p1_) + bool(p2_)) {
     case 0:
       __throw_filesystem_error(what, ec);
     case 1:
       __throw_filesystem_error(what, *p1_, ec);
     case 2:
       __throw_filesystem_error(what, *p1_, *p2_, ec);
     }
     _LIBCPP_UNREACHABLE();
   }
 
-  T report(errc const& err) const { return report(make_error_code(err)); }
+  _LIBCPP_FORMAT_PRINTF(3, 4)
+  T report(const error_code& ec, const char* msg, ...) const {
+    va_list ap;
+    va_start(ap, msg);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    try {
+#endif  // _LIBCPP_NO_EXCEPTIONS
+      report_impl(ec, msg, ap);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    } catch (...) {
+      va_end(ap);
+      throw;
+    }
+#endif  // _LIBCPP_NO_EXCEPTIONS
+    va_end(ap);
+    return error_value<T>();
+  }
+
+  T report(errc const& err) const {
+    return report(make_error_code(err));
+  }
 
-  template <class... Args>
-  T report(errc const& err, const char* msg, Args const&... args) const {
-    return report(make_error_code(err), msg, args...);
+  _LIBCPP_FORMAT_PRINTF(3, 4)
+  T report(errc const& err, const char* msg, ...) const {
+    va_list ap;
+    va_start(ap, msg);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    try {
+#endif  // _LIBCPP_NO_EXCEPTIONS
+      report_impl(make_error_code(err), msg, ap);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    } catch (...) {
+      va_end(ap);
+      throw;
+    }
+#endif  // _LIBCPP_NO_EXCEPTIONS
+    va_end(ap);
+    return error_value<T>();
   }
 
 private:
   ErrorHandler(ErrorHandler const&) = delete;
   ErrorHandler& operator=(ErrorHandler const&) = delete;
 };
 
 using chrono::duration;

libcxx/src/filesystem/operations.cpp

#else
   timeval tv;
   gettimeofday(&tv, 0);
   return time_point(__secs(tv.tv_sec) + __microsecs(tv.tv_usec));
 #endif // CLOCK_REALTIME
 }
 
 filesystem_error::~filesystem_error() {}
 
-#if defined(_LIBCPP_WIN32API)
-#define PS_FMT "%ls"
-#else
-#define PS_FMT "%s"
-#endif
-
 void filesystem_error::__create_what(int __num_paths) {
   const char* derived_what = system_error::what();
   __storage_->__what_ = [&]() -> string {
-    const path::value_type* p1 = path1().native().empty() ? PS("\"\"") : path1().c_str();
-    const path::value_type* p2 = path2().native().empty() ? PS("\"\"") : path2().c_str();
     switch (__num_paths) {
-    default:
+    case 0:
       return detail::format_string("filesystem error: %s", derived_what);
     case 1:
-      return detail::format_string("filesystem error: %s [" PS_FMT "]", derived_what,
-                                   p1);
+      return detail::format_string("filesystem error: %s [" PATH_CSTR_FMT "]",
+                                   derived_what, path1().c_str());
     case 2:
-      return detail::format_string("filesystem error: %s [" PS_FMT "] [" PS_FMT "]",
-                                   derived_what, p1, p2);
+      return detail::format_string("filesystem error: %s [" PATH_CSTR_FMT "] [" PATH_CSTR_FMT "]",
+                                   derived_what, path1().c_str(), path2().c_str());
     }
+    _LIBCPP_UNREACHABLE();
   }();
 }
 
 static path __do_absolute(const path& p, path* cwd, error_code* ec) {
   if (ec)
     ec->clear();
   if (p.is_absolute())
     return p;
#else
   if (ret == nullptr)
     ret = "/tmp";
 
   path p(ret);
 #endif
   error_code m_ec;
   file_status st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
-    return err.report(m_ec, "cannot access path \"" PS_FMT "\"", p);
+    return err.report(m_ec, "cannot access path " PATH_CSTR_FMT, p.c_str());
 
   if (!exists(st) || !is_directory(st))
-    return err.report(errc::not_a_directory, "path \"" PS_FMT "\" is not a directory",
-                      p);
+    return err.report(errc::not_a_directory,
+                      "path " PATH_CSTR_FMT " is not a directory", p.c_str());
 
   return p;
 }
 
 path __weakly_canonical(const path& p, error_code* ec) {
   ErrorHandler<path> err("weakly_canonical", ec, &p);
 
   if (p.empty())

libcxx/test/support/filesystem_test_helper.h

struct ExceptionChecker {
   void check_libcxx_string(fs::filesystem_error const& Err) {
     std::string message = std::make_error_code(expected_err).message();
 
     std::string additional_msg = "";
     if (!opt_message.empty()) {
       additional_msg = opt_message + ": ";
     }
     auto transform_path = [](const fs::path& p) {
-      if (p.native().empty())
-        return std::string("\"\"");
-      return p.string();
+      return "\"" + p.string() + "\"";
     };
     std::string format = [&]() -> std::string {
       switch (num_paths) {
       case 0:
         return format_string("filesystem error: in %s: %s%s", func_name,
                              additional_msg, message);
       case 1:
         return format_string("filesystem error: in %s: %s%s [%s]", func_name,