[PATCH] D27679: WIP: simplify format member detection in FormatVariadic

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/Support/FormatVariadicDetails.h:22-23
 namespace detail {
+class format_adapter {};
+}
+
----------------
labath wrote:
> zturner wrote:
> > What if we added `virtual void format(llvm::raw_ostream &Stream, StringRef Style) const = 0;` to this class?
> > 
> > This would guaranteed that if the `is_base_of` check ever succeeded, it would never fail to compile because of a missing format member.
> I wasn't sure how much we cared about the performance impact of a virtual call. I can certainly add it. (Note the code will still fail to compile if the member is missing. The abstract function will give the error message a bit earlier though.)
If we mark all adapters `final`, then there should never be a virtual function call.  The performance impact probably won't be a big deal, but I also don't see a reason not to do this.


================
Comment at: include/llvm/Support/FormatVariadicDetails.h:25-32
+template<typename T>
+class format_adapter: public detail::format_adapter {
+  typedef T type;
+protected:
+  explicit format_adapter(T Item) : Item(Item) {}
+
+  T Item;
----------------
labath wrote:
> zturner wrote:
> > Do we actually need the templated version of this class?  Why not just have `AdapterBase` inherit directly from `format_adapter`?  It seems like `AdapterBase<T>` and `format_adapter<T>` could be merged into 1 class.
> Yeah, that can certainly be cleaned up. I am not sure I like the name AdapterBase though. If this is going to be the main entry point for user-written code, I'd rather call it FormatAdapter or Adapter (be it in CamelCase or snake_form).
`FormatAdapter` sounds fine.  BTW, we may not even need any other base class at all.  the `is_base_of` could check for `is_base_of<T, FormatAdapter<T>>` and we could reduce it down to just 1 base class.


https://reviews.llvm.org/D27679