[FR]: EXPECT_THROW, EXPECT_NO_THROW, ASSERT_THROW, and ASSERT_NO_THROW don't play nice with [[nodiscard]]

[Jacobfaib]

Does the feature exist in the most recent commit?

Tested on v1.13.0, but the same problem also exists at head:

googletest/googletest/include/gtest/internal/gtest-internal.h

Lines 1367 to 1373 in 79219e2

	#define GTEST_TEST_THROW_(statement, expected_exception, fail) \
	GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
	if (::testing::internal::TrueWithString gtest_msg{}) { \
	bool gtest_caught_expected = false; \
	try { \
	GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement); \
	} catch (expected_exception const&) { \

where GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_() expands as

googletest/googletest/include/gtest/internal/gtest-internal.h

Lines 1312 to 1316 in 79219e2

	#define GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement) \
	if (::testing::internal::AlwaysTrue()) { \
	statement; \
	} else /* NOLINT */ \
	static_assert(true, "") // User must have a semicolon after expansion.

Why do we need this feature?

I can work around it by doing

[[nodiscard]] int a_function_that_throws();

ASSERT_THROW(static_cast<void>(a_function_that_throws()), ...)

but I feel like gtest should render this nicety for me.

Describe the proposal.

Wrap statement in static_cast<void>(statement) or std::ignore = statement. The former is probably more general.

Is the feature specific to an operating system, compiler, or build system version?

No.

[dkaszews]

I for one would rather ASSERT_NO_THROW not hide the [[nodiscard]].

The statement inside the macro can also be assignment, so you can do:

ASSERT_NO_THROW(std::ignore = a_function_that_throws());

// or
int result{};
ASSERT_NO_THROW(result = a_function_that_throws());
EXPECT_EQ(result, 1);

[Jacobfaib]

@dkaszews Fair, I an argument could be made for *_NO_THROW not to gobble the value -- I suspect it will come down to personal preference :). The main issue really is the *_THROW family, I've updated the original post to focus more on that.

In the throwing case, the return value is truly irrelevant, so gtest should handle that gracefully for us.

[dkaszews]

@Jacobfaib Sure, EXPECT_THROW effectively means ECPECT_DOES_NOT_RETURN. I would be fine with gtest silencing the [[nodiscard]] in that case.

Question is, are throwing functions with [[nodiscard]] really that common? I find it a weird combination, because I have always only seen [[nodiscard]] used either with pure nothrow functions, or functions which return error codes and therefore also never throw.

The only case I can thing of is something like std::string::substr which should be marked as [[nodiscard]] (but isn't in standard) to indicate it returns a new object instead of modifying the old one, while could still throw on out of bounds index.

[Jacobfaib]

Question is, are throwing functions with [[nodiscard]] really that common?

Very common, at least in my experience. We use [[nodiscard]] liberally in our code bases, since if the function returns something then 9/10 the user should be capturing that return value (or explicitly choose to ignore it). Whether a function returns something or throws exceptions are orthogonal concepts.

For example, you can have a factory function that validates the inputs before constructing the widget. This is both [[nodiscard]] and throwing.