rollbear / trompeloeil

Header only C++14 mocking framework
Boost Software License 1.0
805 stars 86 forks source link

WITH with this causes warning: implicit capture of 'this' with recent clangs #342

Open SimonKagstrom opened 2 days ago

SimonKagstrom commented 2 days ago

With a recent Apple clang update (Apple clang version 16.0.0 (clang-1600.0.26.3)) we've started getting deprecation warnings for some unit tests, related to how the trompeloeil WITH macro is used:

<source>:27:47: warning: implicit capture of 'this' with a capture default of '=' is deprecated [-Wdeprecated-this-capture]

A short example to reproduce this with regular clang-17 (https://godbolt.org/z/x6YKzb6Gb):

#include <doctest.h>
#include <doctest/trompeloeil.hpp>

using trompeloeil::_;

namespace
{

class Fixture
{
public:
    MAKE_MOCK1(MyMock, void(int));
    int my_side_effect = 0;

    int Helper(int i)
    {
        return i;
    }
};

} // namespace

TEST_CASE_FIXTURE(Fixture, "WITH side effect test")
{
    // error: implicit capture of 'this' with a capture default of '=' is deprecated [-Werror,-Wdeprecated-this-capture]
    REQUIRE_CALL(*this, MyMock(_)).WITH(_1 == Helper(_1));
    // Works:
    // REQUIRE_CALL(*this, MyMock(_)).LR_WITH(_1 == Helper(_1));

    MyMock(5);
}

As in the example, we can use LR_WITH to correct this. However, capturing this with [&] seems unnecessary, so is there some trompeloeil change that should be done for this, or should we start using LR_WITH for these?

rollbear commented 1 day ago

I am surprised that it has taken this long for anyone to report this. Yes, the implicit capture of this with [=] is deprecated since C++20. Unfortunately I really don't know what to do about it. This problem arises when test frameworks implement tests in member functions. A way around this would be to change the macro to capture [=,this], but then sources wouldn't compile for test frameworks that do not implement tests as member functions since there is no this to capture, and those that do implement tests as member functions would still give warnings if the test doesn't in fact access any member.

Suggestions are very welcome.

SimonKagstrom commented 1 day ago

Would it be possible to special-case this for e.g., doctest and catch2 (I guess), if doctest/trompeloeil.hpp is included? I.e., with some e.g., ifdef so [=,this] in that case?

For other people encountering this and using cmake+conan, I've worked around the issue by disabling the warning for Trompeloeil and it's dependencies:

target_compile_options(trompeloeil::trompeloeil
INTERFACE
    -Wno-deprecated-this-capture
)
rollbear commented 1 day ago

Maybe possible, but not easily. Catch2, and I believe doctest also, only implements tests as member functions when you use fixtures, otherwise they are freestanding functions. So the macro would have to expand to different things depending on where it is expanded.

SimonKagstrom commented 1 day ago

Looking at the metaprogramming library and constraints and concept pages on cppreference, I get a feeling there's a solution to this hiding there somewhere. :-)

AndrewPaxie commented 17 hours ago

How does a C++ library writer respond to an existential threat from the C++ committee? This warning could become a hard error as soon as C++26. Nobody said breaking backward compatibility in C++ would make life easier for library writers! /rant

@SimonKagstrom : I believe there is too, starting with whether decltype(EXPR) is well-formed or not in the same context the WITH appears, where EXPR is the argument to WITH. SFINAE might tell us which expressions are ill-formed: those are the expressions that might work if a lambda capture containing this is provided. The well-formed ones get a lambda with just [=] capture as current code has. There is the case where the EXPR remains ill-formed even after trying to form a lambda with a [=,this] capture, but those WITH clauses are ill-formed in the current implementation anyway. All this mechanism is active for C++20 or later. Business as usual for previous language dialects.

Of course we remain reliant on C++ compiler maintainers to write conforming implementations, that is not to remove support for a construct for previous language dialects "by accident", but that has always been the a part of this life. /rant

Sorry to be so sketchy here, I would have liked to present an implementation. I am hopeful @rollbear or yourself can see what I am talking about.