[googletest] Avoid Clang warning about known-NULL condition variables when jumping into else statements (issue1175041)

[chan...@gmail.com]

Reviewers: Jeffrey Yasskin, Zhanyong Wan,

Message:
Hey,

When working on googletest, this warning spews like crazy from Clang. We
discussed making the warning detect goto/label edges and behave
correctly, but it complicates the warning and would likely slow down
Clang to do this.

At first, I suspected it would be really hard to re-write this macros to
avoid the warning, but I got the idea to just add a layer between the
variable declaring if-statement and the control-flow managing
if-statement. It might be possible to factor this more, but I'm not sure
it's worth it.

Thoughts?



Please review this at http://codereview.appspot.com/1175041/show

Affected files:
M include/gtest/internal/gtest-internal.h


Index: include/gtest/internal/gtest-internal.h
===================================================================
--- include/gtest/internal/gtest-internal.h (revision 423)
+++ include/gtest/internal/gtest-internal.h (working copy)
@@ -808,7 +808,9 @@

#define GTEST_TEST_THROW_(statement, expected_exception, fail) \
GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
- if (const char* gtest_msg = "") { \
+ if (const char* gtest_msg = "") \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (true) { \
bool gtest_caught_expected = false; \
try { \
GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement); \
@@ -833,7 +835,9 @@

#define GTEST_TEST_NO_THROW_(statement, fail) \
GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
- if (const char* gtest_msg = "") { \
+ if (const char* gtest_msg = "") \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (true) { \
try { \
GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement); \
} \
@@ -848,7 +852,9 @@

#define GTEST_TEST_ANY_THROW_(statement, fail) \
GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
- if (const char* gtest_msg = "") { \
+ if (const char* gtest_msg = "") \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (true) { \
bool gtest_caught_any = false; \
try { \
GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement); \
@@ -880,7 +886,9 @@

#define GTEST_TEST_NO_FATAL_FAILURE_(statement, fail) \
GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
- if (const char* gtest_msg = "") { \
+ if (const char* gtest_msg = "") \
+ GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
+ if (true) { \
::testing::internal::HasNewFatalFailureHelper
gtest_fatal_failure_checker; \
GTEST_SUPPRESS_UNREACHABLE_CODE_WARNING_BELOW_(statement); \
if (gtest_fatal_failure_checker.has_new_fatal_failure()) { \

[jyas...@gmail.com]

http://codereview.appspot.com/1175041/diff/1/2
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/1175041/diff/1/2#newcode811
include/gtest/internal/gtest-internal.h:811: if (const char* gtest_msg =
"") \
Another workaround for this would be to define a ConstCharP struct that
wraps the const char* and converts back implicitly. The clang warning
doesn't fire on structs.

If someone writes:
if (cond)
EXPECT_THROW(...) << "stuff";
else
other_stuff();

which if does that else bind to? Is there a test for that?

http://codereview.appspot.com/1175041/show

[w...@google.com]

http://codereview.appspot.com/1175041/diff/1/2
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/1175041/diff/1/2#newcode811
include/gtest/internal/gtest-internal.h:811: if (const char* gtest_msg =
"") \

On 2010/05/10 04:07:53, Jeffrey Yasskin wrote:

> If someone writes:
> if (cond)
> EXPECT_THROW(...) << "stuff";
> else
> other_stuff();

> which if does that else bind to? Is there a test for that?

It's supposed to bind to "if (cond)". gtest_unittest.cc contains some
tests for this sort of things, but they may not be complete enough to
cover this.

The EXPECT_THROW() macro should behave like a single, atomic statement.
Therefore it cannot expand to an unbalanced 'if' statement. I have an
idea on how to fix the Clang warning without sacrificing the properties
of EXPECT_THROW() or using a helper class. Will send out a patch soon.

http://codereview.appspot.com/1175041/show

[Zhanyong Wan (λx.x x)]

Unfortunately, my idea doesn't work for EXPECT_THROW(). Clang seems
really stubborn about the waring. It works for the rest of the
exception assertions through. In the end, I think we still need the
helper class Jeffrey suggested.

--
Zhanyong

[Chandler Carruth]

If I see one more false positive from this warning, I'll disable it in google. =[ Jeffrey has a slew of complaints against it, marginalizing its benefit in my eyes. We'll see if it rears its head again or if upstream actually fixes it. Thanks for working around it here, will help when reducing bugs out of googletest.

 

--
Zhanyong