Improve format member detection in llvm::formatv

[labath]

Summary

The existing detection of a format member function has a couple of deficiencies:

• the member function does not get detected if one calls formatv with an lvalue, because the template parameter gets deduced as T&, which fails the is_class check.
• it also did not work if the function was called with a const variable because the template parameter would get deduced as const T&, again failing the is_class check.

This fixes the problem by stripping the references in the uses_format_member
template, to make sure the type is correctly detected as class. It also provides
specializations of the has_FormatMember template for const and non-const members
of the types in order to enable declaring the format member as a "const"
function. I have added tests that verify that formatv can be now called in these
scenarios. As some scenarios could not be verified at runtime (e.g. making sure
that calling a non-const format member on a const object does *not* compile), I
have also added some static_asserts which test the behaviour of the template
classes used internally by formatv().

[zturner]

Looks good, thanks!

llvm/trunk/include/llvm/Support/FormatVariadicDetails.h

 };
 
 template <typename T> class missing_format_wrapper;
 
 // Test if T is a class that contains a member function with the signature:
 //
 // void format(raw_ostream &, StringRef);
 //
+// It is assumed T is a non-reference type.
 template <class T, class Enable = void> class has_FormatMember {
 public:
   static bool const value = false;
 };
 
 template <class T>
 class has_FormatMember<T,
-                       typename std::enable_if<std::is_class<T>::value>::type> {
-  using Signature_format = void (T::*)(llvm::raw_ostream &S, StringRef Options);
+                       typename std::enable_if<std::is_class<T>::value &&
+                                               std::is_const<T>::value>::type> {
+  using CleanT = typename std::remove_volatile<T>::type;
+  using Signature_format = void (CleanT::*)(llvm::raw_ostream &S,
+                                            StringRef Options) const;
 
   template <typename U>
   static char test2(SameType<Signature_format, &U::format> *);
 
   template <typename U> static double test2(...);
 
 public:
-  static bool const value = (sizeof(test2<T>(nullptr)) == 1);
+  static bool const value = (sizeof(test2<CleanT>(nullptr)) == 1);
+};
+
+template <class T>
+class has_FormatMember<
+    T, typename std::enable_if<std::is_class<T>::value &&
+                               !std::is_const<T>::value>::type> {
+  using CleanT = typename std::remove_cv<T>::type;
+  using Signature_format = void (CleanT::*)(llvm::raw_ostream &S,
+                                            StringRef Options);
+
+  template <typename U>
+  static char test2(SameType<Signature_format, &U::format> *);
+
+  template <typename U> static double test2(...);
+
+public:
+  static bool const value =
+      (sizeof(test2<CleanT>(nullptr)) == 1) || has_FormatMember<const T>::value;
 };
 
 // Test if format_provider<T> is defined on T and contains a member function
 // with the signature:
 //   static void format(const T&, raw_stream &, StringRef);
 //
 template <class T> class has_FormatProvider {
 public:
public:
   static bool const value =
       (sizeof(test<llvm::format_provider<Decayed>>(nullptr)) == 1);
 };
 
 // Simple template that decides whether a type T should use the member-function
 // based format() invocation.
 template <typename T>
 struct uses_format_member
-    : public std::integral_constant<bool, has_FormatMember<T>::value> {};
+    : public std::integral_constant<
+          bool,
+          has_FormatMember<typename std::remove_reference<T>::type>::value> {};
 
 // Simple template that decides whether a type T should use the format_provider
 // based format() invocation.  The member function takes priority, so this test
 // will only be true if there is not ALSO a format member.
 template <typename T>
 struct uses_format_provider
-    : public std::integral_constant<bool, !has_FormatMember<T>::value &&
-                                              has_FormatProvider<T>::value> {};
+    : public std::integral_constant<
+          bool, !uses_format_member<T>::value && has_FormatProvider<T>::value> {
+};
 
 // Simple template that decides whether a type T has neither a member-function
 // nor format_provider based implementation that it can use.  Mostly used so
 // that the compiler spits out a nice diagnostic when a type with no format
 // implementation can be located.
 template <typename T>
 struct uses_missing_provider
-    : public std::integral_constant<bool, !has_FormatMember<T>::value &&
-                                              !has_FormatProvider<T>::value> {};
+    : public std::integral_constant<bool,
+                                    !uses_format_member<T>::value &&
+                                        !uses_format_provider<T>::value> {};
 
 template <typename T>
 typename std::enable_if<uses_format_member<T>::value,
                         member_format_wrapper<T>>::type
 build_format_wrapper(T &&Item) {
   return member_format_wrapper<T>(std::forward<T>(Item));
 }
 

llvm/trunk/unittests/Support/FormatVariadicTest.cpp

 //===- FormatVariadicTest.cpp - Unit tests for string formatting ----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
+// Compile-time tests for the uses_format_member template
+namespace {
+struct ConstFormat {
+  void format(raw_ostream &OS, StringRef Opt) const { OS << "ConstFormat"; }
+};
+
+struct Format {
+  void format(raw_ostream &OS, StringRef Opt) { OS << "Format"; }
+};
+
+using detail::uses_format_member;
+
+static_assert(uses_format_member<Format>::value, "");
+static_assert(uses_format_member<Format &>::value, "");
+static_assert(uses_format_member<Format &&>::value, "");
+static_assert(not uses_format_member<const Format>::value, "");
+static_assert(not uses_format_member<const Format &>::value, "");
+static_assert(not uses_format_member<const volatile Format>::value, "");
+static_assert(not uses_format_member<const volatile Format &>::value, "");
+
+static_assert(uses_format_member<ConstFormat>::value, "");
+static_assert(uses_format_member<ConstFormat &>::value, "");
+static_assert(uses_format_member<ConstFormat &&>::value, "");
+static_assert(uses_format_member<const ConstFormat>::value, "");
+static_assert(uses_format_member<const ConstFormat &>::value, "");
+static_assert(uses_format_member<const volatile ConstFormat>::value, "");
+static_assert(uses_format_member<const volatile ConstFormat &>::value, "");
+}
+
 TEST(FormatVariadicTest, EmptyFormatString) {
   auto Replacements = formatv_object_base::parseFormatString("");
   EXPECT_EQ(0U, Replacements.size());
 }
 
 TEST(FormatVariadicTest, NoReplacements) {
   const StringRef kFormatString = "This is a test";
   auto Replacements = formatv_object_base::parseFormatString(kFormatString);

 }
 
 TEST(FormatVariadicTest, Adapter) {
   class Negative {
     int N;
 
   public:
     explicit Negative(int N) : N(N) {}
-    void format(raw_ostream &S, StringRef Options) { S << -N; }
+    void format(raw_ostream &S, StringRef Options) const { S << -N; }
   };
 
   EXPECT_EQ("-7", formatv("{0}", Negative(7)).str());
 
   int N = 171;
 
   EXPECT_EQ("  171  ",
             formatv("{0}", fmt_align(N, AlignStyle::Center, 7)).str());
   EXPECT_EQ(" 171   ", formatv("{0}", fmt_pad(N, 1, 3)).str());
   EXPECT_EQ("171171171171171", formatv("{0}", fmt_repeat(N, 5)).str());
 
   EXPECT_EQ(" ABABABABAB   ",
             formatv("{0:X-}", fmt_pad(fmt_repeat(N, 5), 1, 3)).str());
   EXPECT_EQ("   AB    AB    AB    AB    AB     ",
             formatv("{0,=34:X-}", fmt_repeat(fmt_pad(N, 1, 3), 5)).str());
 }
 
 TEST(FormatVariadicTest, ImplicitConversions) {
   std::string S = formatv("{0} {1}", 1, 2);
   EXPECT_EQ("1 2", S);
 
   SmallString<4> S2 = formatv("{0} {1}", 1, 2);
   EXPECT_EQ("1 2", S2);
-}
+}
+
+TEST(FormatVariadicTest, FormatMember) {
+  EXPECT_EQ("Format", formatv("{0}", Format()).str());
+
+  Format var;
+  EXPECT_EQ("Format", formatv("{0}", var).str());
+  EXPECT_EQ("Format", formatv("{0}", std::move(var)).str());
+
+  // Not supposed to compile
+  // const Format cvar{};
+  // EXPECT_EQ("Format", formatv("{0}", cvar).str());
+}
+
+TEST(FormatVariadicTest, FormatMemberConst) {
+  EXPECT_EQ("ConstFormat", formatv("{0}", ConstFormat()).str());
+
+  ConstFormat var;
+  EXPECT_EQ("ConstFormat", formatv("{0}", var).str());
+  EXPECT_EQ("ConstFormat", formatv("{0}", std::move(var)).str());
+
+  const ConstFormat cvar{};
+  EXPECT_EQ("ConstFormat", formatv("{0}", cvar).str());
+}