Page MenuHomePhabricator

Simplify format member detection in FormatVariadic
ClosedPublic

Authored by labath on Dec 12 2016, 9:32 AM.

Details

Summary

This replaces the format member search, which was quite complicated, with a more
direct approach to detecting whether a class should be formatted using the
format-member method. Instead we use a special type llvm::format_adapter, which
every adapter must inherit from. Then the search can be simply implemented with
the is_base_of type trait.

Aside from the simplification, I like this way more because it makes it more
explicit that you are supposed to use this type only for adapter-like
formattings, and the other approach (format_provider overloads) should be used
as a default (a mistake I made when first trying to use this library).

The only slight change in behaviour here is that now choose the format-adapter
branch even if the format member invocation will fail to compile (e.g. because it is a
non-const member function and we are passing a const adapter), whereas
previously we would have gone on to search for format_providers for the type.
However, I think that is actually a good thing, as it probably means the
programmer did something wrong.

Event Timeline

labath updated this revision to Diff 81100.Dec 12 2016, 9:32 AM
labath retitled this revision from to WIP: simplify format member detection in FormatVariadic.
labath updated this object.
labath added reviewers: zturner, inglorion.
labath added a subscriber: llvm-commits.
zturner added inline comments.Dec 12 2016, 9:41 AM
include/llvm/Support/FormatVariadicDetails.h
22–23 ↗(On Diff #81405)

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.

25–32 ↗(On Diff #81405)

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.

labath planned changes to this revision.Dec 12 2016, 10:02 AM

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..

include/llvm/Support/FormatVariadicDetails.h
22–23 ↗(On Diff #81405)

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.)

25–32 ↗(On Diff #81405)

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).

zturner added inline comments.Dec 12 2016, 10:23 AM
include/llvm/Support/FormatVariadicDetails.h
22–23 ↗(On Diff #81405)

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.

25–32 ↗(On Diff #81405)

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.

labath updated this revision to Diff 81205.Dec 13 2016, 2:41 AM
  • More cleanup

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.

include/llvm/Support/FormatVariadicDetails.h
25–32 ↗(On Diff #81405)

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.

labath updated this revision to Diff 81379.Dec 14 2016, 7:41 AM

Update documentation.

labath retitled this revision from WIP: simplify format member detection in FormatVariadic to Simplify format member detection in FormatVariadic.Dec 14 2016, 7:41 AM
labath updated this object.

I think I've done all the cleanup I wanted to do here. Let me know what you think.

labath updated this revision to Diff 81395.Dec 14 2016, 8:57 AM
labath updated this object.

Running the unittests for the latest version discovered a new issue. The usage
of && in the FormatAdapter base class forces each class deriving from it to use
std::move. I am not sure how much we care about that, but I am going to look
more into how we can avoid that.

zturner edited edge metadata.Dec 14 2016, 9:40 AM

Can you confirm that after your changes, we still trigger the missing_format_adapter error message if you try to format a type that doesn't have a formatter?

For example, just make a dummy class Foo and try to pass an instance of it to formatv. Make sure that the error message says something about missing_format_adapter<T> where T = 'Foo'.

labath updated this revision to Diff 81405.Dec 14 2016, 9:57 AM
labath edited edge metadata.

Yes, I can. I can also add a static_assert test for that.

As for the std::move issue, the only two options (which do not involve massive
template magic) I can think of are:

  • live with it
  • have the base class not store the formatted member and leave it up for each derived class to decide how to store it.
zturner accepted this revision.Dec 14 2016, 11:23 AM
zturner edited edge metadata.

I see that you added the missing format provider test. Can you also just verify by inspection that the error message is still nice when it fails? (You may have already done this, but there's no way to really write a test for it). Assuming that's fine and all the tests pass, then lgtm.

This revision is now accepted and ready to land.Dec 14 2016, 11:23 AM

I wouldn't call any template-related error "nice", but yes, I have tried it, and it's the same error as before.

This revision was automatically updated to reflect the committed changes.