Avoid Clang warning about known-NULL condition variables when jumping into else statements

[chandlerc, 2010-05-09 23:55:46]

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?

[Jeffrey Yasskin, 2010-05-10 04:07:53]

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?

[wan, 2010-05-10 07:04:44]

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.

[wan, 2010-05-10 07:44:43]

On Mon, May 10, 2010 at 12:04 AM,  <wan@google.com> wrote:
>
> 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.

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

[chandlerc, 2010-05-10 08:02:13]

On Mon, May 10, 2010 at 12:44 AM, Zhanyong Wan (λx.x x) <wan@google.com>wrote:

> On Mon, May 10, 2010 at 12:04 AM,  <wan@google.com> wrote:
> >
> > 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.
>
> 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.
>

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
>