Add Catch::toString support for containers

[mat-tso]

In order to enhance REQUIRE and CHECK error message when testing standard containers, add overloads of Catch::toString for:

• std::pair
• std::deque
• std::list
• std::array (c++11)
• std::forward_list (c++11)
• (more types upcoming)
• any type that has a container api (no need to add support for each container individually)

Those types were printed as "{?}" (default toString implementation for unsupported class). This was contradictory with the documentation:

"Most [...] std types are supported out of the box"

when in fact only string, vector and tupple were supported.

Detail:

• Renamed the toStringVector.cpp test file to toStringContainers.cpp and type parametrized the vector tests to run them also for deque and list.
• The overhead of including all the standard container headers is negligable. => No longer needed, types are treated as containers if they fulfill (some) of the container concept constraints.
• Types are consider containers if they contain value_type and const_iterator members and have begin and end support (members or ADL findable) returning a const_iterator. const_iterator::operator* must also return a const value_type &
• Beware that a trying to printing a type fulfilling those requirements but returning invalid iterators will results in undefined behaviour. In such case specialize the Catch::IsContainer trait to contain static const bool value = false in order revert to the default behaviour (printing "{?}").

[mat-tso]

In the first patch I move the toString(pair<...>) overload from the test to the public header.

• Have I missed something, why has it never been done before ? Is their a flaw in the current toString(pair<...>) design ?

This is a first draft although I hope to eventually add support for all C++ containers.

• But first I would like some feedback about my current way of factorizing the tests ? Usually I tend to put several layers of template everywhere 😄 ; so I forced myself to make it simple here.
• Is their any (experimental ?) way to add typed catch test ? That would feel like the canonical way to go.

[mat-tso]

Remove comment, patch not done yet.

[mat-tso]

An other drawback of the Catch::toString implementation is that catch.hpp will include every container header which may or may not impact significantly build time for tests not using lots of different containers. Nevertheless I could not find a way to avoid that. The only thing I thought off was to forward declare the containers but it is undefined behaviour to "adds declarations or definitions to namespace std".

Those extra inclusion might not have a big impact tough. I'll need to benchmark it.

[mat-tso]

After some extensive testing, my conclusion is that the inclusion and overload for all those containers is negligible. I could not measure a > 4ms overhead.

[mat-tso]

To implement all 17 standard containers, it might be more readable to use a temporary macro. Though the template commas are going to be quite painful to handle.

[Trass3r]

A generic container toString with enable_if begin/end-checks does not work?
Would also remove the need to include all the headers.

template <typename T>
std::string toString(const T& c )
{
    return Detail::rangeToString(c.begin(), c.end());
}

[mat-tso]

While I was adding a catch_tostring.cpp file to the SurrogateCpps I noticed that those surrogate cpps were only compiled in the xcode build system. Is their a technical reason for that ? I might as well add them to the cmake config as I am using it to compile the tests.

[nabijaczleweli]

Why not

template<class Container>
std::string toString(Container const& c) {
    return Detail::rangeToString( begin(c), end(c) );
}

This oughta catch 90% of the cases at least

[mat-tso]

@nabijaczleweli Your proposition would work if toString was only formatting containers. Unfortunately, toString has overloads to format basic types, tuples, pairs and the user can overload it to add his custom types.
Thus template<class Container> std::string toString(Container const& c); overload would be ambiguous with any other template overload. C++ has rules to reduce the viable function set, but it would still be quite dangerously ambiguous.

One solution to reduce this ambiguity would be to use some good old SFINAL:

/// Port some C++11 features
namespace detail {
/// C++11 enable_if
template<bool B, class T = void>
struct enable_if {};

template<class T>
struct enable_if<true, T> { typedef T type; };

/// C++11 declval
template <class T>
T &declval();
} // namespace detail

/** Only allow this overload to participate to overload resolution
 * if calling `begin` and `end` on a `Container` instance
 * does not result in an error.
 */
template<class Container>
detail::enable_if<sizeof(begin(internal::declval<Container>)) ==
                  sizeof(end(internal::declval<Container>)),
std::string> toString(Container const& c) {
        return Detail::rangeToString( begin(c), end(c) );
}

This would restrict the toString overload to types with a begin and end support. Nevertheless that is still dangerous:

• compilers errors are more complex to understand
• the user might have defined a class with begin and end support. It does not mean it follows the container concept. Eg: a ChessGame class might have a begin and end method that are starting and stopping the game, not returning iterators.
• I can not nor want to predict the outcome of the partial ordering rules if the user overloads toString for his custom container.

To conclude, I do not think that an opt out strategic is a good idea. Still, an opt in declaration could work.
Something like:

// Got lazy, this is C++11  ;)

/// Types are not by default containers
template <class T>
struct IsContainer : std::false_type {};

/// Declare which class are containers
template <class T, class A>
struct IsContainer<std::vector<T, A>> : std::true_type {};
template <class T, class A>
struct IsContainer<std::list<T, A>> : std::true_type {};
...
template <class Ts...>
struct IsContainer<std::map<Ts...>> : std::true_type {};

template<class Container>
std::enable_if<IsContainer<Container>::value,
std::string> toString(Container const& c) {
        return Detail::rangeToString( begin(c), end(c) );
}

Note that all containers headers must still be included.

User could opt in for his custom container as such:

namespace Catch { // specialization must occur in primary declaration namespace
struct IsContainer<USER_CUSTOM_CONTAINER_TYPE> : std::true_type {};
}

What is your opinion ?

[nabijaczleweli]

With the second approach you're still facing a fundamental problem: you need to list every container type. I'd go for a modified approach 1: either (a) require, that there are ADL-accessible begin(c) and end(c), or require every part of the Container concept to be satisfied.

the user might have defined a class with begin and end support. It does not mean it follows the container concept. Eg: a ChessGame class might have a begin and end method that are starting and stopping the game, not returning iterators.

Note: those are begin(c), not c.begin(), user needs to add theirnamespace::begin() (and toString() implementation needs to use std::begin;) for it to work with stdlib ranged-for and generic algorithms. Same for end().

[Trass3r]

I'd also go with range-concept checks.

compilers errors are more complex to understand

clang and gcc error messages have become quite good, at least when enable_if is in the template parameter list and not in the return type which is generally preferable anyway.

the user might have defined a class with begin and end support. It does not mean it follows the container concept.

Such a corner case must not impede progress.

[mat-tso]

the user might have defined a class with begin and end support. It does not mean it follows the container concept.

Such a corner case must not impede progress.

If that is the general consensus, then it will make the implementation very generic and lightweight. Could the maintainer confirm that such corner case should not be handled by design ?

@philsquared: Could you confirm that undefined behaviour is acceptable when printing a class with a begin and end method (or ADL accessible) BUT not returning proper iterators.

Eg: an instance with begin and end returning nullptr would segfault the program when trying to pretty print it.

[lightmare]

How about

template<class Container>
typename std::enable_if<std::is_base_of<std::forward_iterator_tag,
    typename std::iterator_traits<typename Container::iterator>::iterator_category>::value,
std::string>::type toString(Container const& c)
...

That should rule out non-containers.

[mat-tso]

As far as I know forward_iterator_tag is only an optimization trick, custom iterators are not required to derived from it.
I am currently testing that the container has a value_type and const_iterator members and that begin and end call on the container are returning a const_iterator.
I'll push that code very soon. I currently have some annoying overload ambiguity to fix.

[mat-tso]

Implemented the container auto detection as discussed. It was more complicated than expected, mostly because meta-programing in C++03 is quite harder than with the current standard. Had to remember sizeof tricks I stopped using a long time ago. 👴
Please review ! 👍 The most complex part is the container detection (IsContainer class) code, check it twice. I am open for style remarks too, it is the first time I contribute in Catch.

[Trass3r]

Nice 👍
To me a C++11-only feature in #ifdefs would have also been fine. This is not a bug preventing Catch from working.
In general I'd vote for setting a C++11 minimum requirement. gcc will make 14 the default soon and in msvc you can't even disable it.
There could still be a C++03 maintenance branch if really necessary.

[mat-tso]

Fixed coding style, and factorized TrueType/FalseType.

To me a C++11-only feature in #ifdefs would have also been fine.
My only fear for pre c++11 support is old compiler with broken sfinae support.
Nevertheless sfinae is already used to detect stream << support. A bit more should not hurt.
EDIT: stream support detection do NOT use sfinae but a clever low prio operator<< to avoid it. See my next comment.

I tested the code on windows with cmake (vs2015). 2 tests are broken but not due to my patches, master also has those tests fail. I'll try to fix them.
Are their other platforms officially supported but not tested by the CI ?

[philsquared]

Just catching up here!
The complexities involved here are the main reason I never got around to extending the container support before.
Nice work on getting to where you are. I'll take a closer look a bit later.
As for SFINAE, I did experiment with an SFINAE version of << detection but (a) it's opt in (you have to #define something) and (b) I found that, on average, it was no better than the non-SFINAE version anyway.
That said, because there is an SFINAE #define already you could use that if you want to conditionally compile.

As for C++11 baselining I'm doing this for Catch2, which is already started and I hope to be pushing to a public repo soon(ish).

[philsquared]

oh - and I accepted a PR to fix those failing travis builds just recently (if it's the same one - master is building cleanly now). You might just need to rebase?

[Trass3r]

I'd pull it out of the return type.

template <typename C,
          typename = typename Detail::enable_if<Detail::IsContainer<C>::value, void>::type>
std::string toStringInternal(const C& c)

[mat-tso]

Unfortunately, function template default parameters were introduced in c++11. Your proposition would not compile.

[Trass3r]

Already forgotten about that. Haven't written C++03 in years :D

[mat-tso]

Added support for c arrays. Their sized are deduced at compile time to array therefore any decay would fall back on the plain old pointer display.

[mat-tso]

I can no think about any other kind of container to add support for. Consider this PR feature complete !

[mat-tso]

Spook too soon, C++14 is broken due to overload conflicts between the cbegin&cend added to support pre c++11 and those coming from the C++14 standard library.

[mat-tso]

Found the source of my overload probleme: begin and end can not be declare in Catch if compiled in c++11 as that would lead to overload ambiguity with the stl one find via ADL. Thus they is now a guard "NOT C++11" for those function.

Also had to restrict my sfinae expression to check for ++ and != support for c++11 only. This troubles me a bit as SFINAE expression are known to be unsupported even by compiler claiming to be c++11 compliant. I will test it on windows and either remove it or use the catch all technique already used to detect operator<<(stream) support.

[Trass3r]

If you mean msvc here's their online compiler: http://webcompiler.cloudapp.net/

[mat-tso]

There is still a problem with begin and end. They are defined only in C++11 to avoid ambiguity with the standard library but on osx, although the C++11 flag is not set, std::begin and std::end ARE defined.

One solution would be to weaken my begin and end overloads. Maybe by using a custom conversion.

Nevertheless the code is already quite complicated. Instead I going to DROP the support for C++03. Only std::vector in C++03 as it is the case on master (no regression).

Converting the PR to C++11 will make the patches a lot easier to read and maintain.

[mat-tso]

Dropped C++03 support of the container detection. Made the code a lot smaller! Hopefully Travis will validate my PR this time. A last review before merge ?

[mat-tso]

There seems to be a difference of type on osx of *begin(vector<bool>{true}). It calls the toString(bool) overload on all platform except osx for which the template<class T> toString(T) overload is called. I will try to fix that next week.

[stefanhaller]

What are the chances of getting this updated for Catch2? I don't think I will have much time to help drive this forward, but it would sure be a useful feature to have.

[philsquared]

I've been having a look at this today and had a go at coming up with something myself. I wanted something with fewer bits of template machinery, if possible, and what I've come up with is something that just tests if there's an overload of the begin and end functions (either in std, the global namespace, or as found by ADL) for the type. I realise this leaves open the possibility that such overloads exist, yet the type is not a container, but I'm hopeful this is not a problem in practice (in the event that it does get reported as a legitimate issue I know there are other ways to achieve the same thing).

Now we can fully assume C++11 this is much easier. Here's a mock up of what I'm thinking:

https://godbolt.org/g/5Jn7Gm

The linked godbolt shows it running against a number of types on a handful of representative compilers. I've also tried this on VS2017 (haven't had a chance on 2015, yet, but I'm reasonably confident).

Note that this version explicitly detects arrays as well. I've also put a top level specialisation for string in, as that would otherwise be detected as a containers - but we want to handle that differently. In Catch2 that already has a specialisation of StringMaker<> which plays that role.
Any comments from those still watching this before I see how this rolls into Catch2 (btw, as part of that I'll be watching the impact on compile times, since one of my worries with this is that it would take a hit. One nice property of this, however, is that it doesn't require any of the containers to be included in Catch2 itself. At the moment we have a load of #defines to minimise the impact of doing so).

[mat-tso]

That looks good. Unfortunately, I do not have the time to do the port but can review it if you want.
I do not think the amount of container check I coded were justified in practice.
Instead, it could be documented that if a type is wrongly detected as container, the user can specialize is_container to be false.

[mat-tso]

Note that c++11 begin and end support c-array so the specialisation line 19 should not be needed. You need to provide an lvalue to begin and end though.
https://godbolt.org/g/SDRGmQ

[mat-tso]

BTW, feel free to use my Unit tests (or any other part of my CL), https://github.com/catchorg/Catch2/pull/606/files#diff-43562f40f8c6dcfe2c54557316e0f852. They were pretty helpfull to find inconsistency (bool, bool[], vector<bool>... especialy)

[philsquared]

@mat-tso - don't worry, I'll be doing the porting part - very happy for you to review, though.
As for specialising is_container<>, I don't anticipate having that level in the Catch2 version - rather its role will be taken by StringMaker<>. Your point about users specialising it stand, though - with the added bonus they get to specific how it gets serialised at the same time :-)
I'll take a look at your tests and see what I can borrow - thanks!

[philsquared]

Oh, and wrt the array detection - I didn't think of using declval<T&> to make it detect automatically. I might use that. OTOH there might be some merit in keeping array detection separate, too. I'll think about that.

[philsquared]

Ok, I've checked in my changes in the "stringify" branch for now.
I've benchmarked the compile times and there seems to be a slight improvement as result! (and master is a slight improvement over the current single include as well) - so that's good news!

[horenmar]

v2.1.0 is out and with it the support for stringification of ranges (things that respond to requests for iterators).