Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHECK and REQUIRE can't expand a custom type (QString) #531

Closed
vpicaver opened this issue Oct 29, 2015 · 24 comments
Closed

CHECK and REQUIRE can't expand a custom type (QString) #531

vpicaver opened this issue Oct 29, 2015 · 24 comments
Labels

Comments

@vpicaver
Copy link

I've implementing operator<< and Catch::toString for printing QString's in CHECK statements but nothing seems to work. QString is a string class from the Qt framework.

#include "catch.hpp"

std::ostream& operator << ( std::ostream& os, QString const& value ) {
    os << value.toStdString();
    return os;
}

namespace Catch {
    std::string toString( QVariant const& value ) {
        return value.toString().toStdString();
    }
}

TEST_CASE("My testcase")
{
    CAPTURE(QString("moo"));
    INFO(QString("Sauce"));

    CHECK(QString("moo") == QString("sauce"));

This will prints out:

/Users/vpicaver/Documents/Projects/cavewhere/testcases/SurveyChunkTest.cpp:91: FAILED:
  CHECK( QString("moo") == QString("sauce") )
with expansion:
  {?} == {?}
with messages:
  QString("moo") := moo
  Sauce

CAPTURE and INFO work okay, but the expansion still returns {?} == {?}

I also try to add #define CATCH_CONFIG_SFINAE it didn't seem to help.

Is this a bug? Or am I doing something wrong?

@nabijaczleweli
Copy link
Contributor

#define CATCH_CONFIG_MAIN
#include "catch.hpp"

struct QString {
    std::string _;
    const std::string & toStdString() const { return _; }

    bool operator==(const QString & rhs) { return _ == rhs._; }
};

std::ostream& operator <<( std::ostream& os, QString const& value ) {
    os << value.toStdString();
    return os;
}

TEST_CASE("My testcase") {
  CAPTURE(QString{"moo"});
  INFO(QString{"Sauce"});

  CHECK(QString{"moo"} == QString{"sauce"});
}

Works for me (test code copied verbatim, added mock QString):

-------------------------------------------------------------------------------
My testcase
-------------------------------------------------------------------------------
asdf.cpp:28
...............................................................................

asdf.cpp:32: FAILED:
  CHECK( QString{"moo"} == QString{"sauce"} )
with expansion:
  moo == sauce
with messages:
  QString{"moo"} := moo
  Sauce

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

@vpicaver
Copy link
Author

This does work with your mock QString, but doesn't work with the real QString from the Qt library. QString implements a bunch of operator<< and I'm not sure if it's effecting catch.

@nabijaczleweli
Copy link
Contributor

Please provide an SSCCE, because you're doing something quite fishy by the looks of it

@vpicaver
Copy link
Author

Sure no problem. What platform are you on? So I can provide instructions. Are you ever used the Qt framework?

@nabijaczleweli
Copy link
Contributor

The example should be platform-independent (I'm on Windows) Also, try not to use Qt to achieve the minimal code sample to reproduce the bug

@elken
Copy link

elken commented Nov 4, 2015

You can cast a QString to a std::string with QString::toLocal8Bit().constData().

This passes. It's kinda dirty & the string loses encoding (you have to use QTextCodec::codecForLocale() to handle unicode), but it works at least.

@vpicaver
Copy link
Author

vpicaver commented Nov 4, 2015

On that note we could probably use QString().toStdString() instead.

@elken
Copy link

elken commented Nov 5, 2015

For sure. We have QString::fromStdString, not having one for the other direction seems wrong.

@jedwards1211
Copy link

@vpicaver look who it is!!! :)

I just stumbled on this too

@jedwards1211
Copy link

@vpicaver I found these operators already defined in our project in SurveyChunkTest.cpp:

std::ostream& operator << ( std::ostream& os, QString const& value ) {
    os << value.toStdString();
    return os;
}

std::ostream& operator << ( std::ostream& os, QVariant const& value ) {
    os << value.toString();
    return os;
}

Because of these, when I tried to implement them in dewalls test cases, I got a duplicate symbol error from the linker. (of course that seems horribly wrong, it doesn't seem right for qbs to build the main project modules into my submodule tests)

Did you have the same error? Maybe that's why you thought some of the operator<< that QString implements was causing an issue?

I checked in QString and it only implements operator<< for QDataStream. QTextStream also implements an operator<< for QString but I don't see how any of this would interfere with std::ostream?

@jedwards1211
Copy link

@vpicaver alas you were right. Even when I built dewalls separately with the operators defined, it didn't work.

@jedwards1211
Copy link

Is it just me or do C++ compilers have major problems picking the right overloaded method in some cases? I've seen C++ compilers get confused about overloaded methods that Java never would

@nabijaczleweli
Copy link
Contributor

that Java never would

cough Read The International Standard if you think the compilers are getting confused. And no, you haven't seen them get confused.
Fun activity for an afternoon: compare what The International Standard has to say about function overloading to what that filthy Java standard says.

@jedwards1211
Copy link

I'm not doubting that the standard is correct. But it's been long known that many compilers don't completely conform to the standard, or at least have had bugs at some point. For instance: http://stackoverflow.com/questions/25126987/intel-c-compiler-bug-in-member-function-overload-resolution-involving-using
And I take it back about Java, I did see the standard Java compiler get confused once, when the Eclipse JDT compiler didn't, in some case with complicated generic typing.

Given that QString has no inheritance hierarchy nor templates, and that the operator<< works if I std::cout << QString("hello world") in a test case, I'm surprised it wouldn't work for expansion.

@jedwards1211
Copy link

Aha, I found something that works!

So in catch.hpp it says to overload toString, not to specialize its template:

/// Overload (not specialise) this template for custom typs that you don't want
/// to provide an ostream overload for.
template<typename T>
std::string toString( T const& value ) {
    return StringMaker<T>::convert( value );
}

But lo and behold, specializing the template worked for me:
tostrings.h:

#ifndef TOSTRINGS_H
#define TOSTRINGS_H

#include "../lib/catch.hpp"

#include <string>
#include <QString>

namespace Catch {
template<> std::string toString<QString>(QString const& s);
}

#endif // TOSTRINGS_H

tostrings.cpp:

#include "tostrings.h"

namespace Catch {
template<> std::string toString<QString>(QString const& s) {
    return s.toLocal8Bit().constData();
}
}

In a test file, I #include "tostrings.h" and do the following:

    CHECK( QString("a") == QString("b") );

Output:

/Users/andy/cavewhere/dewalls/test/wallsprojectparsertests.cpp:121: FAILED:
  CHECK( QString("a") == QString("b") )
with expansion:
  a == b

@vpicaver
Copy link
Author

vpicaver commented Dec 5, 2015

Sweet, I need to try that out! It's weird because I thought I implemented a
tostring function but I couldn't get it to work.

On Saturday, December 5, 2015, Andy Edwards notifications@github.com
wrote:

Aha, I found something that works!

So in catch.hpp it says to overload toString, not to specialize its
template:

/// Overload (not specialise) this template for custom typs that you don't want/// to provide an ostream overload for.template
std::string toString( T const& value ) {
return StringMaker::convert( value );
}

But lo and behold, specializing the template worked for me:
tostrings.h:

#ifndef TOSTRINGS_H
#define TOSTRINGS_H

#include "../lib/catch.hpp"

#include
#include
namespace Catch {template<> std::string toString(QString const& s);
}

#endif // TOSTRINGS_H

tostrings.cpp:

#include "tostrings.h"
namespace Catch {template<> std::string toString(QString const& s) {
return s.toLocal8Bit().constData();
}
}

In a test file, I #include "tostrings.h" and do the following:

CHECK( QString("a") == QString("b") );

Output:

/Users/andy/cavewhere/dewalls/test/wallsprojectparsertests.cpp:121: FAILED:
CHECK( QString("a") == QString("b") )
with expansion:
a == b


Reply to this email directly or view it on GitHub
#531 (comment).

@jedwards1211
Copy link

@vpicaver yeah, well as I said, implementing it as an overload didn't work...only specializing the template (despite that catch.hpp says not to) worked for me

@jedwards1211
Copy link

Well, specializing the template didn't work in MSVC2013, the Linker complained that there were multiple symbols for that function.
@nabijaczleweli this is what I'm talking about with compilers getting confused. Whatever the standard might say, compilers don't always behave the same way.

@philsquared
Copy link
Collaborator

I put the comment there about overloading, rather than specialising, because (a) being consistent seemed more reliable in general and (b) overloading seemed to work in cases that specialising didn't for me (although I never fully understood why in those cases). Overloading tends to be more forgiving too, and is generally the preferred approach.
If specialising works for you great! It may not do so portably, though (depending on what the underlying issue is that stopping overloading from working.

@jedwards1211 re the multiple symbols - did you mark your specialisation as inline?

Anyway, instead of specialising toString() you might want to try specialising the StringMaker template struct: https://github.com/philsquared/Catch/blob/master/docs/tostring.md#catchstringmaker-specialisation

@jedwards1211
Copy link

@philsquared no, for some reason I did it non-inline...I'll have to investigate later if that caused problems, can't remember.

Would you say that if it doesn't work with a given compiler/linker, then it means there's a bug in the compiler?

@philsquared
Copy link
Collaborator

TBH this is an area I get a little hazy on (perhaps @PureAbstract will drop in to clear things up?).

But I believe there are two ways this can trip up: (1) is to do with two-phase lookup and so non-conformance in this area (I'm looking at you, MSVC) may yield different results than a conforming compiler (and so it would be fair to classify it as a bug - albeit one of, known, non-conformance). But (2) is to do with an aspect of implementation defined behaviour that governs the timing of when dependent instantiations are fully resolved. This is the one I'm hazier on - I don't even remember the details well enough to describe it now - I just know I've hit it in the past where different compilers may end up putting templates together in different ways - or not compiling - but all in adherence to the standard! I don't know whether that one is at play here - more likely to be 2PL.

@PureAbstract
Copy link
Contributor

Well, since you ask...my preference is to specialise the StringMaker template rather than playing around trying to get the right set of overloads/specialisations of toString.

There are a bunch of issues here (I made some detailed notes on this a while back but can't seem to lay my hands on them right now) - but Herb Sutter wrote an article a while back that summarises most of them quite nicely, and gives a pretty clear rationale for preferring specialising the (class|struct) template.

http://www.gotw.ca/publications/mill17.htm

@jedwards1211
Copy link

@PureAbstract that is a top notch article, and taught me for the first time about which function is chosen out of a jumble of overloads and specializations.

Yet it leaves me wondering more than ever why explicitly overloading toString(QString) doesn't work, since overloads should always get top priority.

@horenmar
Copy link
Member

horenmar commented Jun 8, 2017

I am going to close this, because Catch::toString is currently deprecated and is going away in favour of specializing Catch::StringMaker in Catch 2.0, which at this point is under development and thus might actually happen reasonably soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants