Project does not compile with Clang about approvaltests.cpp HOT 12 CLOSED

Hi @claremacrae, I've just opened an issue (just one minute before your last comment) about the support for fmt library.

from approvaltests.cpp.

Hi @alepez - great minds thing alike!!!!

from approvaltests.cpp.

Thank you for a helpful and comprehensive bug report. Really appreciated.

I'll read through the links, and next time Llewellyn and I pair on this library, we'll review the options...

I'll also set up a clang build in Windows on my PC, to reproduce the build failure - and see if I can do the same in our CI builds.

Again, many thanks for recording this.

from approvaltests.cpp.

I've been able to reproduce this on Windows, using WSL Ubuntu 18.04 and clang 6.0, and seen that the forward declaration fixes the compilation.

From an initial chat earlier today, we are intending to add in the forward declaration for now, in CombinationTests.cpp, with a comment explaining why, and then documenting the requirement to declare it before including the Approvals header.

That would mean we will be doing the same as the first section here:
https://github.com/catchorg/Catch2/blob/master/docs/tostring.md#operator--overload-for-stdostream

I would suggest that we then create a separate ticket that covers providing a customization point.

from approvaltests.cpp.

Sounds like a good plan! 👍

from approvaltests.cpp.

Hi Alastair,

We've fixed the compilation in clang, and also added some documentation of the issue:

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/ToString.md

Do you think that covers enough about customising the output formats in Approvals. Feedback welcome...

I'm unsure whether this now shows enough about how customisable the string output already is, or whether you feel it's still worth opening a separate tip...

Thanks again for reporting the issue!

from approvaltests.cpp.

Hi Clare,

Sorry it's taken some time to get back to you on this. The new documentation is definitely helpful, thanks. Something that I missed the first time around was that verifyAll can take a converter function object for its third argument. Until I realised that, I was under the impression that callers had no choice but to provide a type with an operator<< overload. I hadn't considered the possibility of writing a wrapper either, so it's good to see that documented.

I think it would be good to explicitly document the use of the converter argument to verifyAll. I know it's used in the tests, but when I initially encountered the ApprovalTests.cpp library (after attending your excellent C++ on Sea talk), the first thing I did was to try and compile the project, and only when I ran in to the clang compile error did I start reading the documentation. It's only now that I've delved into the tests that I see what the intended usage is.

On the subject of wrapper objects, I wonder if there is any benefit in also showing a version which takes a reference to the wrapped object, rather than copying it? Not sure if that's really necessary, and obviously there are additional lifetime issues to consider when using it. It's just something that struck me whilst reading the new example. In the past I've tried to use Golden Master testing with bits of legacy code where there was a 'God Object' containing significant amounts of state. Copying one of those is often undesirable or not possible.

ie.:

struct FormatRectangleForMultipleLines{

    explicit FormatRectangleForMultipleLines(Rectangle3 const& rectangle) : rectangle(&rectangle)
    {
    }

    friend std::ostream &operator<<(std::ostream &os, const FormatRectangleForMultipleLines &wrapper) {
        os << "(x,y,width,height) = (" <<
           wrapper.rectangle->x << "," <<
           wrapper.rectangle->y << "," <<
           wrapper.rectangle->width << "," <<
           wrapper.rectangle->height << ")";
        return os;
    }

  private:
    Rectangle3 const* rectangle; // Non-owning pointer
};

from approvaltests.cpp.

Sorry it's taken some time to get back to you on this. The new documentation is definitely helpful, thanks. Something that I missed the first time around was that verifyAll can take a converter function object for its third argument. Until I realised that, I was under the impression that callers had no choice but to provide a type with an operator<< overload. I hadn't considered the possibility of writing a wrapper either, so it's good to see that documented.

This comment made me realise that whilst verifyAll can take a converter function, verify cannot. So I've added that to the todo list as a high priority.

I think it would be good to explicitly document the use of the converter argument to verifyAll. I know it's used in the tests, but when I initially encountered the ApprovalTests.cpp library (after attending your excellent C++ on Sea talk), the first thing I did was to try and compile the project, and only when I ran in to the clang compile error did I start reading the documentation. It's only now that I've delved into the tests that I see what the intended usage is.

Sure - I've added improving of documentation here to the todo list too.

On the subject of wrapper objects, I wonder if there is any benefit in also showing a version which takes a reference to the wrapped object, rather than copying it? Not sure if that's really necessary, and obviously there are additional lifetime issues to consider when using it. It's just something that struck me whilst reading the new example. In the past I've tried to use Golden Master testing with bits of legacy code where there was a 'God Object' containing significant amounts of state. Copying one of those is often undesirable or not possible.

Good point - I've just changed FormatRectangleForMultipleLines so that it stores a const reference to the wrapped object.

from approvaltests.cpp.

Tentatively closing this ticket, as the tests do now build and pass on clang, and the documentation is much improved.

(There are some other actions for further improvements, which we'll do in due course, but they feel like separate things now, to me.)

Thanks again @aharrison24 for the bug report and suggestions - really appreciated!

from approvaltests.cpp.

I’m re-opening this having re-read it, to make sure I re-review the suggestions in the light of the current code, and of things I’ve learned since first reading it.

from approvaltests.cpp.

In comments on Stack Overflow question "Convention on printing STL containers for approval tests", @alepez said:

I've forked your library to make some experiments. Here I'm using fmt to automatically format supported containers: https://github.com/alepez/ApprovalTests.cpp/blob/fmt-ranges/ApprovalTests/utilities/StringUtils.h#L59-L72

This is a test project: https://github.com/alepez/approvaltests-experiments/blob/master/tests/experiments.cpp I've specialized StringUtils::toString instead of implementing operator<<, so it doesn't collide with possible other implementations of operator<< for std containers

This is tremendous - if I understand it correctly, it is a big step towards an implementation of the kind of StringMaker functionality provided by Catch2, as in this Catch2QtStringMaker.h example - but already fitting in with the ApprovalTests.cpp...

I knew we needed something like this, but I didn't know that it could be possible using existing code in ApprovalTests.cpp!!!

from approvaltests.cpp.

I'm probably going outside the scope of this Issue now, but I believe a better approach would be to provide a customization point that allows users to provide custom string conversions for their own types, or for types that do not easily support a stream insertion operator. Sometimes third-party types do provide a stream insertion operator, but its output isn't acceptable for some reason. In those cases, it's helpful to have a separate mechanism for stringifying the type.

This customization point for users to provide their own string converter is now available, So we are going to close the issue, if you believe this is done in error, please let us know

Thanks for the issue

from approvaltests.cpp.