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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

If we add the virtual functions, then this class begins to look a lot like `format_wrapper`, to the point of format_wrapper being obsolete. I tried to simplify this more by removing the wrapper class and using adapters everywhere instead. The basic logic of the formatv function then becomes: "You can either pass in a FormatAdapter directly, or if you don't, we'll create one for you using the format_provider specializations."

The side effect of the virtual function is that it forces us to choose between a const and a non-const version. I think that formatters generally should be deterministic ( == const), but I suppose there could be some occasional situations where they could maintain some aggregate state. So I choose the non-const version, as it is more flexible, but I could be convinced to change it.



================
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;
----------------
zturner wrote:
> 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.
That will not work because at this point, `T` is the `FormatAdapter<RealT>`. It may be possible to do some strange recursion here `is_base_of<FormatAdapter<typename T::type>, T>`, but I think it's not worth it, especially as having a common base type enables other simplifications.


https://reviews.llvm.org/D27679