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

REQUIRE_THROWS_AS does not support class constructors #216

Closed
jktjkt opened this issue Mar 27, 2019 · 5 comments
Closed

REQUIRE_THROWS_AS does not support class constructors #216

jktjkt opened this issue Mar 27, 2019 · 5 comments

Comments

@jktjkt
Copy link
Contributor

jktjkt commented Mar 27, 2019

Description

I would like to use the REQUIRE_THROWS_AS macro with class constructors, not just freestanding functions. The following code results in a build failure for me (GCC 8.1.0, Doctest from the latest dev):

#define FAKE_READING(X) \
  // this is our internal macro which calls into Trompeloeil, it mocks
  // a virtual TTY as the variable named "serial"

DOCTEST_SUBCASE("nothing on the tty") {
    FAKE_READING("");
    REQUIRE_THROWS_AS(DriverUnderTest(serial), FramingError);
}

The error:

foo.cpp: In function ‘void _DOCTEST_ANON_FUNC_2()’:
foo.cpp:175:36: error: unnecessary parentheses in declaration of ‘serial’ [-Werror=parentheses]
         REQUIRE_THROWS_AS(DriverUnderTest(serial), FramingError);
                                          ^
foo.cpp:175:43: error: no matching function for call to ‘some::driver::DriverUnderTest::DriverUnderTest()’
         REQUIRE_THROWS_AS(DriverUnderTest(serial), FramingError);

I know that in this particular scneario I could workaround this via initializing via the uniform construction syntax with curly braces, i.e., DriverUnderTest{serial}, but that's not possible in general because Foo{x, y} and Foo(x, y) are different constructors (see std::vector::vector for example).

  • doctest version: latest dev
  • Operating System: Linux
  • Compiler+version: GCC 8.1.0
@onqtam
Copy link
Member

onqtam commented Mar 27, 2019

well... you could do what I suggested in this commit in a PR to json: nlohmann/json@5d511a6

basically: have a compatibility header which includes the actual doctest header and have the following in there:

#include <doctest/doctest.h>

#undef CHECK_THROWS
#undef CHECK_THROWS_AS
#undef CHECK_THROWS_WITH
#undef CHECK_NOTHROW

#undef REQUIRE_THROWS
#undef REQUIRE_THROWS_AS
#undef REQUIRE_THROWS_WITH
#undef REQUIRE_NOTHROW

// doctest allows multiple statements in these macros (even blocks of code) but json
// tests rely on passing single function/constructor calls which have a [[nodiscard]]
// attribute so here we static_cast to void - just like Catch does
#define CHECK_THROWS(expr) DOCTEST_CHECK_THROWS(static_cast<void>(expr))
#define CHECK_THROWS_AS(expr, e) DOCTEST_CHECK_THROWS_AS(static_cast<void>(expr), e)
#define CHECK_THROWS_WITH(expr, e) DOCTEST_CHECK_THROWS_WITH(static_cast<void>(expr), e)
#define CHECK_NOTHROW(expr) DOCTEST_CHECK_NOTHROW(static_cast<void>(expr))

#define REQUIRE_THROWS(expr) DOCTEST_REQUIRE_THROWS(static_cast<void>(expr))
#define REQUIRE_THROWS_AS(expr, e) DOCTEST_REQUIRE_THROWS_AS(static_cast<void>(expr), e)
#define REQUIRE_THROWS_WITH(expr, e) DOCTEST_REQUIRE_THROWS_WITH(static_cast<void>(expr), e)
#define REQUIRE_NOTHROW(expr) DOCTEST_REQUIRE_NOTHROW(static_cast<void>(expr))

I don't think REQUIRE_THROWS_AS() will be changed by default - perhaps a config option, or a separate macro, or just a link to this issue in the docs... not really sure what the right path is

@jktjkt
Copy link
Contributor Author

jktjkt commented Mar 27, 2019

Thanks for your quick reply. I have to admit that I do not fully understand how a [[nodiscard]] is relevant here. Neither my constructors nor the class itself are marked that way, and this isn't implicit for constructors as far as I can tell.

Looking further, this looks like an instance of the C++'s most vexing parse. And indeed, here's what clang says:

error: parentheses were disambiguated as redundant parentheses around declaration of variable named
      'serial' [-Werror,-Wvexing-parse]
        REQUIRE_THROWS_AS(FooModule(serial), OperationError);
                                   ^~~~~~~~

I wonder what your motivation for not casting is here. To me it is quite reasonable to assert that a particular construction fails...

@onqtam
Copy link
Member

onqtam commented Mar 28, 2019

The idea is to support arbitrary code - like entire blocks or multiple statements:

REQUIRE_THROWS_AS(if (x) { foo(); bar(); }, ex_type);
REQUIRE_THROWS_AS(foo(); bar(), ex_type);

But when I think about it - users could just use a lambda - this was a concern when doctest supported C++98...

So if I change this it would be a breaking change........... And yes - nodiscard has nothing to do but the casting will do the trick, right?

@jktjkt
Copy link
Contributor Author

jktjkt commented Mar 28, 2019

Confirming that static_cast<void> is a workaround.

@onqtam
Copy link
Member

onqtam commented Apr 6, 2019

6 days ago I added the DOCTEST_CONFIG_VOID_CAST_EXPRESSIONS config option - when you define it before the doctest header you would get the desired behavior. Currently only in the dev branch - will release an official version... soon (1-2 weeks probably).

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

No branches or pull requests

2 participants