Test MakeArg and fix formatting.

format.h

@@ -191,9 +191,16 @@ class BasicStringRef {
     Returns the string size.
    */
   std::size_t size() const {
-    if (size_ == 0) size_ = std::char_traits<Char>::length(data_);
+    if (size_ == 0 && data_) size_ = std::char_traits<Char>::length(data_);
     return size_;
   }
+
+  friend bool operator==(BasicStringRef lhs, BasicStringRef rhs) {
+    return lhs.data_ == rhs.data_;
+  }
+  friend bool operator!=(BasicStringRef lhs, BasicStringRef rhs) {
+    return lhs.data_ != rhs.data_;
+  }
 };
 
 typedef BasicStringRef<char> StringRef;
@@ -611,17 +618,29 @@ struct Arg {
 template <typename Char>
 class MakeArg : public Arg {
  private:
-  // This method is private to disallow formatting of arbitrary pointers.
+  // The following two methods are private to disallow formatting of
-  // If you want to output a pointer cast it to const void*. Do not implement!
+  // arbitrary pointers. If you want to output a pointer cast it to
+  // "void *" or "const void *". In particular, this forbids formatting
+  // of "[const] volatile char *" which is printed as bool by iostreams.
+  // Do not implement!
   template <typename T>
   MakeArg(const T *value);
-
-  // This method is private to disallow formatting of arbitrary pointers.
-  // If you want to output a pointer cast it to void*. Do not implement!
   template <typename T>
   MakeArg(T *value);
 
- public:
+  void SetString(StringRef str) {
+    type = STRING;
+    string.value = str.c_str();
+    string.size = str.size();
+  }
+
+  void SetString(WStringRef str) {
+    type = WSTRING;
+    wstring.value = str.c_str();
+    wstring.size = str.size();
+  }
+
+public:
   MakeArg() {}
   MakeArg(bool value) { type = INT; int_value = value; }
   MakeArg(short value) { type = INT; int_value = value; }
@@ -629,6 +648,8 @@ class MakeArg : public Arg {
   MakeArg(int value) { type = INT; int_value = value; }
   MakeArg(unsigned value) { type = UINT; uint_value = value; }
   MakeArg(long value) {
+    // To minimize the number of types we need to deal with, long is
+    // translated either to int or to long long depending on its size.
     if (sizeof(long) == sizeof(int)) {
       type = INT;
       int_value = static_cast<int>(value);
@@ -659,38 +680,18 @@ class MakeArg : public Arg {
     int_value = internal::CharTraits<Char>::ConvertChar(value);
   }
 
-  MakeArg(const char *value) {
+  MakeArg(char *value) { SetString(value); }
-    type = STRING;
+  MakeArg(const char *value) { SetString(value); }
-    string.value = value;
+  MakeArg(const std::string &value) { SetString(value); }
-    string.size = 0;
+  MakeArg(StringRef value) { SetString(value); }
-  }
 
-  MakeArg(const wchar_t *value) {
+  MakeArg(wchar_t *value) { SetString(value); }
-    type = WSTRING;
+  MakeArg(const wchar_t *value) { SetString(value); }
-    wstring.value = value;
+  MakeArg(const std::wstring &value) { SetString(value); }
-    wstring.size = 0;
+  MakeArg(WStringRef value) { SetString(value); }
-  }
 
-  MakeArg(Char *value) {
-    type = STRING;
-    string.value = value;
-    string.size = 0;
-  }
-
-  MakeArg(const void *value) { type = POINTER; pointer_value = value; }
   MakeArg(void *value) { type = POINTER; pointer_value = value; }
-
+  MakeArg(const void *value) { type = POINTER; pointer_value = value; }
-  MakeArg(const std::basic_string<Char> &value) {
-    type = STRING;
-    string.value = value.c_str();
-    string.size = value.size();
-  }
-
-  MakeArg(BasicStringRef<Char> value) {
-    type = STRING;
-    string.value = value.c_str();
-    string.size = value.size();
-  }
 
   template <typename T>
   MakeArg(const T &value) {
@@ -1205,7 +1206,7 @@ class BasicWriter {
   void write_str(
       const internal::StringValue<StringChar> &str, const FormatSpec &spec);
 
-    // This method is private to disallow writing a wide string to a
+  // This method is private to disallow writing a wide string to a
   // char stream and vice versa. If you want to print a wide string
   // as a pointer as std::ostream does, cast it to const void*.
   // Do not implement!

posix.h

@@ -77,10 +77,10 @@ namespace fmt {
 
 // An error code.
 class ErrorCode {
-private:
+ private:
   int value_;
 
-public:
+ public:
   explicit ErrorCode(int value = 0) FMT_NOEXCEPT(true) : value_(value) {}
 
   int get() const FMT_NOEXCEPT(true) { return value_; }
@@ -88,14 +88,14 @@ public:
 
 // A buffered file.
 class BufferedFile {
-private:
+ private:
   FILE *file_;
 
   friend class File;
 
   explicit BufferedFile(FILE *f) : file_(f) {}
 
-public:
+ public:
   // Constructs a BufferedFile object which doesn't represent any file.
   BufferedFile() FMT_NOEXCEPT(true) : file_(0) {}
 
@@ -106,7 +106,7 @@ public:
   // Emulate a move constructor and a move assignment operator if rvalue
   // references are not supported.
 
-private:
+ private:
   // A proxy object to emulate a move constructor.
   // It is private to make it impossible call operator Proxy directly.
   struct Proxy {
@@ -178,13 +178,13 @@ public:
 // than an exception. You can get standard behavior by overriding the
 // invalid parameter handler with _set_invalid_parameter_handler.
 class File {
-private:
+ private:
   int fd_;  // File descriptor.
 
   // Constructs a File object with a given descriptor.
   explicit File(int fd) : fd_(fd) {}
 
-public:
+ public:
   // Possible values for the oflag argument to the constructor.
   enum {
     RDONLY = FMT_POSIX(O_RDONLY), // Open for reading only.
@@ -202,14 +202,14 @@ public:
   // Emulate a move constructor and a move assignment operator if rvalue
   // references are not supported.
 
-private:
+ private:
   // A proxy object to emulate a move constructor.
   // It is private to make it impossible call operator Proxy directly.
   struct Proxy {
     int fd;
   };
 
-public:
+ public:
   // A "move constructor" for moving from a temporary.
   File(Proxy p) FMT_NOEXCEPT(true) : fd_(p.fd) {}
 

test/CMakeLists.txt

@@ -29,9 +29,12 @@ expect_compile_error("fmt::internal::Array<char, 5> a, b; b = a;")
 expect_compile_error("const fmt::Writer a, b(a);")
 expect_compile_error("fmt::Writer a, b; b = a;")
 
-# Formatter is not copyable from a temporary.
+# MakeArg doesn't accept [const] volatile char *.
-expect_compile_error("fmt::Formatter<> a(fmt::Formatter<>(\"a\"));")
+expect_compile_error("volatile char s[] = \"test\"; (fmt::internal::MakeArg<char>)(s);")
-expect_compile_error("fmt::Formatter<> b(\"a\"); b = fmt::Formatter<>(\"b\");")
+expect_compile_error("const volatile char s[] = \"test\"; (fmt::internal::MakeArg<char>)(s);")
+
+# MakeArg<char> doesn't accept wchar_t.
+expect_compile_error("fmt::internal::MakeArg<char>(L'a');")
 
 # Writing a wide character to a character stream Writer is forbidden.
 expect_compile_error("fmt::Writer() << L'a';")
@@ -39,7 +42,4 @@ expect_compile_error("fmt::Writer() << fmt::pad(\"abc\", 5, L' ');")
 expect_compile_error("fmt::Writer() << fmt::pad(42, 5, L' ');")
 
 # Formatting a wide character with a narrow format string is forbidden.
-expect_compile_error("fmt::Format(\"{}\") << L'a';")
+expect_compile_error("fmt::format(\"{}\", L'a';")
-
-# There is no implicit conversion from FILE* to FileSink.
-expect_compile_error("fmt::FileSink fs = 0;")

test/util-test.cc

@@ -73,7 +73,7 @@ TEST(UtilTest, Increment) {
 }
 
 #define EXPECT_ARG_(Char, type_code, Type, field, value) { \
-  Type expected_value = value; \
+  Type expected_value = static_cast<Type>(value); \
   fmt::internal::Arg arg = \
     fmt::internal::MakeArg<Char>(expected_value); \
   EXPECT_EQ(fmt::internal::Arg::type_code, arg.type); \
@@ -91,17 +91,17 @@ TEST(UtilTest, MakeArg) {
   EXPECT_ARG(INT, bool, int_value, true);
 
   // Test char.
-  EXPECT_ARG(CHAR, signed char, int_value, 42);
+  EXPECT_ARG(CHAR, signed char, int_value, 'a');
   EXPECT_ARG(CHAR, signed char, int_value, SCHAR_MIN);
   EXPECT_ARG(CHAR, signed char, int_value, SCHAR_MAX);
-  EXPECT_ARG(CHAR, unsigned char, int_value, 42);
+  EXPECT_ARG(CHAR, unsigned char, int_value, 'a');
   EXPECT_ARG(CHAR, unsigned char, int_value, UCHAR_MAX );
   EXPECT_ARG(CHAR, char, int_value, 'a');
   EXPECT_ARG(CHAR, char, int_value, CHAR_MIN);
   EXPECT_ARG(CHAR, char, int_value, CHAR_MAX);
 
   // Test wchar_t.
-  EXPECT_ARGW(CHAR, wchar_t, int_value, 42);
+  EXPECT_ARGW(CHAR, wchar_t, int_value, L'a');
   EXPECT_ARGW(CHAR, wchar_t, int_value, WCHAR_MIN);
   EXPECT_ARGW(CHAR, wchar_t, int_value, WCHAR_MAX);
 
@@ -163,9 +163,17 @@ TEST(UtilTest, MakeArg) {
   char STR[] = "test";
   EXPECT_ARG(STRING, char*, string.value, STR);
   EXPECT_ARG(STRING, const char*, string.value, STR);
-  //EXPECT_ARG(STRING, volatile char*, string.value, STR);
+  EXPECT_ARG(STRING, std::string, string.value, STR);
+  EXPECT_ARG(STRING, fmt::StringRef, string.value, STR);
 
-  // TODO: test pointers
+  // Test wide string.
+  wchar_t WSTR[] = L"test";
+  EXPECT_ARGW(WSTRING, wchar_t*, wstring.value, WSTR);
+  EXPECT_ARGW(WSTRING, const wchar_t*, wstring.value, WSTR);
+  EXPECT_ARGW(WSTRING, std::wstring, wstring.value, WSTR);
+  EXPECT_ARGW(WSTRING, fmt::WStringRef, wstring.value, WSTR);
+
+  // TODO: test that wide string is rejected by MakeArg<char>
 }
 
 // Tests fmt::internal::CountDigits for integer type Int.