YAMLIO: Improve template arg deduction for mapOptional

[labath]

Summary

The way c++ template argument deduction works, both arguments are used
to deduce the template type in the three-argument overload of
mapOptional. This is a problem if the types are slightly different, even
if they are implicitly convertible. This is fairly easy to trigger with
integral types, as the default type of most integral constants is int,
which then requires casting the constant to the type of the other
argument.

This fixes that by telling the compiler to not use the default value for
argument deduction. The "telling" is done by spelling out the argument
type in a more complicated manner, as deduction only happens for simple
types. This allows the default value to be any type which is implicitly
convertible to the value type.

[zturner]

include/llvm/Support/YAMLTraits.h

865–870

What if you just say

template<typename T, typename U>
void mapOptional(const char *Key, T &Val, const U &Default) {
  EmptyContext Ctx;
  mapOptionalWithContext(Key, Val, static_cast<T>(Default), Ctx);
}

This way it's basically self-documenting and doesn't require enable_if magic.

[labath]

The static_cast also allows explicit conversions, which I don't think we
should automatically apply here (that would be circumventing the intention of
the person who made that particular conversion explicit). I fixed that by piping
the default value through a temporary reference. That should allow the same set
of conversions as during a function call, but it is also starting to look a bit
magical.

Let me know what you think.

[labath]

Apply the same fix to mapOptionalWithContext to maintain a consistent interface.

[zturner]

include/llvm/Support/YAMLTraits.h

896

Another possibility is static_assert(std::is_convertible<T, DefaultT>::value, "Default type must be implicitly convertible to value type!"); which should have the same effect, and also yield a better compiler error message. Then, since explicit conversions are disallowed by the static_assert, you could re-write the call site as this->processKeyWithDefault(Key, Val, static_cast<T>(Default), false, Ctx);

I don't have a strong preference, but this feels mildly better to me. WDYT?

[labath]

include/llvm/Support/YAMLTraits.h

896

Yes, that does look better. I'll update the patch with that.

[labath]

Use the static_assert approach to check for convertibility. The only change I
made was to static_cast the default value into a const T & instead of a plain
T, as the latter would force a copy even if the default value was already of
the same type.

(Interestingly, the static_cast to a reference type is already enough to
disqualify explicit conversions for gcc and msvc. Clang still permits those, but
I have no idea who is correct here.)

include/llvm/Support/YAMLTraits.h

public:
   void mapRequired(const char *Key, T &Val, Context &Ctx) {
     this->processKey(Key, Val, true, Ctx);
   }
 
   template <typename T> void mapOptional(const char *Key, T &Val) {
     EmptyContext Ctx;
     mapOptionalWithContext(Key, Val, Ctx);
   }
 
-  template <typename T>
-  void mapOptional(const char *Key, T &Val, const T &Default) {
+  template <typename T, typename DefaultT>
+  void mapOptional(const char *Key, T &Val, const DefaultT &Default) {
     EmptyContext Ctx;
     mapOptionalWithContext(Key, Val, Default, Ctx);
   }
 
   template <typename T, typename Context>
   typename std::enable_if<has_SequenceTraits<T>::value, void>::type
   mapOptionalWithContext(const char *Key, T &Val, Context &Ctx) {
     // omit key/value instead of outputting empty sequence
     if (this->canElideEmptySequence() && !(Val.begin() != Val.end()))
       return;
     this->processKey(Key, Val, false, Ctx);
   }
 
   template <typename T, typename Context>
   void mapOptionalWithContext(const char *Key, Optional<T> &Val, Context &Ctx) {
     this->processKeyWithDefault(Key, Val, Optional<T>(), /*Required=*/false,
                                 Ctx);
   }
 
   template <typename T, typename Context>
   typename std::enable_if<!has_SequenceTraits<T>::value, void>::type
   mapOptionalWithContext(const char *Key, T &Val, Context &Ctx) {
     this->processKey(Key, Val, false, Ctx);
   }
 
-  template <typename T, typename Context>
-  void mapOptionalWithContext(const char *Key, T &Val, const T &Default,
+  template <typename T, typename Context, typename DefaultT>
+  void mapOptionalWithContext(const char *Key, T &Val, const DefaultT &Default,
                               Context &Ctx) {
-    this->processKeyWithDefault(Key, Val, Default, false, Ctx);
+    const T &DefaultAsT = Default; // Use only implicit conversions.
+    this->processKeyWithDefault(Key, Val, DefaultAsT, false, Ctx);
   }
 
 private:
   template <typename T, typename Context>
   void processKeyWithDefault(const char *Key, Optional<T> &Val,
                              const Optional<T> &DefaultValue, bool Required,
                              Context &Ctx) {
     assert(DefaultValue.hasValue() == false &&

unittests/Support/YAMLIOTest.cpp

struct ScalarBitSetTraits<MyFlags> {
     }
   };
   template <>
   struct MappingTraits<FlagsMap> {
     static void mapping(IO &io, FlagsMap& c) {
       io.mapRequired("f1", c.f1);
       io.mapRequired("f2", c.f2);
       io.mapRequired("f3", c.f3);
-      io.mapOptional("f4", c.f4, MyFlags(flagRound));
+      io.mapOptional("f4", c.f4, flagRound);
      }
   };
 }
 }
 
 
 //
 // Test reading flow sequence representing bit-mask values
public:
       uint32_t     hours;
       uint8_t      minutes;
       uint8_t      seconds;
     };
 
     static void mapping(IO &io, TotalSeconds &secs) {
       MappingNormalization<NormalizedSeconds, TotalSeconds> keys(io, secs);
 
-      io.mapOptional("hours",    keys->hours,    (uint32_t)0);
-      io.mapOptional("minutes",  keys->minutes,  (uint8_t)0);
+      io.mapOptional("hours", keys->hours, 0);
+      io.mapOptional("minutes", keys->minutes, 0);
       io.mapRequired("seconds",  keys->seconds);
     }
   };
 }
 }
 
 
 //