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

Zachary Turner via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 12 10:16:29 PST 2016


Yea, sorry, I should have mentioned that I like the simplification of
member detection, as long as the 2-stage lookup is still maintained.  i.e.
1) Search for member, if found use it (even if it is found but doesn't
compile), if not found then 2) Search for format_provider.

On Mon, Dec 12, 2016 at 10:02 AM Pavel Labath via Phabricator <
reviews at reviews.llvm.org> wrote:

> labath planned changes to this revision.
> labath added a comment.
>
> Given that you have commented on implementation details rather than the
> overall idea, I am going to assume you are ok with this approach. I am
> going to refine this and get back..
>
>
>
> ================
> Comment at: include/llvm/Support/FormatVariadicDetails.h:22-23
>  namespace detail {
> +class format_adapter {};
> +}
> +
> ----------------
> 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.)
>
>
> ================
> 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:
> > 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).
>
>
> https://reviews.llvm.org/D27679
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161212/5ba53de0/attachment.html>


More information about the llvm-commits mailing list