Make CHECK_EQ and friends work properly in an if/else structure without braces.

Make CHECK_EQ and friends work properly in an if/else structure without braces.

BUG=

Committed: https://crrev.com/6ad937bc698ce8958d6578609919fe9fd05429f6
Cr-Commit-Position: refs/heads/master@{#339943}

[erikwright (departed), 2015-06-24 15:35:53]

erikwright@chromium.org changed reviewers:
+ danakj@chromium.org

[erikwright (departed), 2015-06-24 15:35:54]

danakj: PTAL.

This fixes the CHECK_EQ family of checks so that they can be used in if/else
blocks without braces.

I also add extensive test coverage for a variety of constructs that these checks
should work in.

[Sigurður Ásgeirsson, 2015-06-25 12:25:09]

siggi@chromium.org changed reviewers:
+ siggi@chromium.org

[Sigurður Ásgeirsson, 2015-06-25 12:25:09]

nice!
These can do with some moar explanation on what's being tested, though.

https://codereview.chromium.org/1193873004/diff/80001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging_unittest.c...
base/logging_unittest.cc:242: bool reached = false;
It's totally opaque what's being tested here and to what end. I dare say that if
this test were to ever fail, I'd be tempted to "fix" the test.
Some running commentary, perhaps?

[danakj, 2015-07-06 22:38:27]

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode505
base/logging.h:505: //   DCHECK_EQ(2, a);
But else if works fine, what is ambiguous?

[Nico, 2015-07-06 22:41:52]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-06 22:41:52]

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode496
base/logging.h:496: };
why is this needed?

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode506
base/logging.h:506: #define CHECK_OP(name, op, val1, val2)                      
            \
isn't the usual pattern for this

  do { /* stuff */ } while (0)

?

[danakj, 2015-07-06 22:43:27]

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode506
base/logging.h:506: #define CHECK_OP(name, op, val1, val2)                      
            \
On 2015/07/06 22:41:52, Nico wrote:
> isn't the usual pattern for this
> 
>   do { /* stuff */ } while (0)
> 
> ?

Ya, that's what I've seen elsewhere too.

[erikwright (departed), 2015-07-07 15:25:28]

On 2015/07/06 22:43:27, danakj wrote:
> https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
> File base/logging.h (right):
> 
>
https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode506
> base/logging.h:506: #define CHECK_OP(name, op, val1, val2)                    
 
>             \
> On 2015/07/06 22:41:52, Nico wrote:
> > isn't the usual pattern for this
> > 
> >   do { /* stuff */ } while (0)
> > 
> > ?
> 
> Ya, that's what I've seen elsewhere too.

That doesn't work because you need the possible '<< "message"' to be part of
'stuff'.

[Nico, 2015-07-07 16:59:44]

I see. Personally, I'd prefer not supporting CHECK in an if without braces over
making the macro goop in this header even harder to read...

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode505
base/logging.h:505: //   DCHECK_EQ(2, a);
On 2015/07/06 22:38:26, danakj wrote:
> But else if works fine, what is ambiguous?

I think the example is wrong and it's supposed to be

  if (a == 1)
    CHECK_EQ(2, a) << "foo";
  else
    statement;

[erikwright (departed), 2015-07-07 17:25:09]

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode496
base/logging.h:496: };
On 2015/07/06 22:41:52, Nico wrote:
> why is this needed?

It allows us to both store the result of logging::Check##name##Impl((val1),
(val2), ...) and also compare it to null.

If the check fails, the result is a string that incorporates the results of
evaluating val1 and val2. Otherwise it is null.

I tried 'if ((std::string* _result = ...) == nullptr) ... else ...' but it does
not work.

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode505
base/logging.h:505: //   DCHECK_EQ(2, a);
On 2015/07/07 16:59:43, Nico wrote:
> On 2015/07/06 22:38:26, danakj wrote:
> > But else if works fine, what is ambiguous?
> 
> I think the example is wrong and it's supposed to be
> 
>   if (a == 1)
>     CHECK_EQ(2, a) << "foo";
>   else
>     statement;

You're correct, the example is wrong. I'm updating the comment and adding a test
case. The compiler error is:

../../base/logging_unittest.cc:424:5: error: add explicit braces to avoid
dangling else [-Werror,-Wdangling-else]
    DCHECK_EQ(false, reached = true);
    ^
../../base/logging.h:723:31: note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../../base/logging.h:700:3: note: expanded from macro 'DCHECK_OP'
  else                                                                \
  ^
1 error generated.

https://codereview.chromium.org/1193873004/diff/80001/base/logging.h#newcode506
base/logging.h:506: #define CHECK_OP(name, op, val1, val2)                      
            \
On 2015/07/06 22:43:27, danakj wrote:
> On 2015/07/06 22:41:52, Nico wrote:
> > isn't the usual pattern for this
> > 
> >   do { /* stuff */ } while (0)
> > 
> > ?
> 
> Ya, that's what I've seen elsewhere too.

That doesn't work because you need the possible '<< "message"' to be part of
'stuff'.

[erikwright (departed), 2015-07-07 18:30:56]

https://codereview.chromium.org/1193873004/diff/80001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/1193873004/diff/80001/base/logging_unittest.c...
base/logging_unittest.cc:242: bool reached = false;
On 2015/06/25 12:25:09, Sigurður Ásgeirsson wrote:
> It's totally opaque what's being tested here and to what end. I dare say that
if
> this test were to ever fail, I'd be tempted to "fix" the test.
> Some running commentary, perhaps?

I've updated this particular test case with some syntactic sugar.

Is it easier to understand now, or does it need commenting still?

Based on your feedback I'll update the others.

[danakj, 2015-07-07 18:39:44]

FWIW if someone wrote

if (a)
  CHECK(b);

I'd ask them to rewrite it as

CHECK_IMPLIES(a, b);

I also don't want to make these macros even harder to read, if this level of
complexity is really needed I don't know that it's worth it.

[erikwright (departed), 2015-07-08 19:22:57]

On 2015/07/07 18:39:44, danakj wrote:
> FWIW if someone wrote
> 
> if (a)
>   CHECK(b);
> 
> I'd ask them to rewrite it as
> 
> CHECK_IMPLIES(a, b);
> 
> I also don't want to make these macros even harder to read, if this level of
> complexity is really needed I don't know that it's worth it.

I've never heard of CHECK_IMPLIES.

If the macros exist they should work as expected anywhere that a statement can
go.  They definitely are used:

git grep "CHECK_\(EQ\|NE\|LE\|LT\|GT\)" | grep -v base/logging.h | grep -v
base/logging_unittest.cc | wc -l
11692

[danakj, 2015-07-08 20:01:22]

On 2015/07/08 19:22:57, erikwright wrote:
> On 2015/07/07 18:39:44, danakj wrote:
> > FWIW if someone wrote
> > 
> > if (a)
> >   CHECK(b);
> > 
> > I'd ask them to rewrite it as
> > 
> > CHECK_IMPLIES(a, b);
> > 
> > I also don't want to make these macros even harder to read, if this level of
> > complexity is really needed I don't know that it's worth it.
> 
> I've never heard of CHECK_IMPLIES.

It's pretty new.

> If the macros exist they should work as expected anywhere that a statement can
> go.  They definitely are used:

Generally, my comment is "put all the code that is for the DCHECK inside the
DCHECK so it will not appear in a release build. if (a) DCHECK(b) leaves the
if(a) outside of the DCHECK which I don't like to see.

> 
> git grep "CHECK_\(EQ\|NE\|LE\|LT\|GT\)" | grep -v base/logging.h | grep -v
> base/logging_unittest.cc | wc -l
> 11692

They are certainly used, I'm not sure what you're arguing here. That they should
be able to be used inside an if()?

[erikwright (departed), 2015-07-09 12:41:55]

On 2015/07/08 20:01:22, danakj wrote:
> On 2015/07/08 19:22:57, erikwright wrote:
> > On 2015/07/07 18:39:44, danakj wrote:
> > > FWIW if someone wrote
> > > 
> > > if (a)
> > >   CHECK(b);
> > > 
> > > I'd ask them to rewrite it as
> > > 
> > > CHECK_IMPLIES(a, b);
> > > 
> > > I also don't want to make these macros even harder to read, if this level
of
> > > complexity is really needed I don't know that it's worth it.
> > 
> > I've never heard of CHECK_IMPLIES.
> 
> It's pretty new.
> 
> > If the macros exist they should work as expected anywhere that a statement
can
> > go.  They definitely are used:
> 
> Generally, my comment is "put all the code that is for the DCHECK inside the
> DCHECK so it will not appear in a release build. if (a) DCHECK(b) leaves the
> if(a) outside of the DCHECK which I don't like to see.
> 
> > 
> > git grep "CHECK_\(EQ\|NE\|LE\|LT\|GT\)" | grep -v base/logging.h | grep -v
> > base/logging_unittest.cc | wc -l
> > 11692
> 
> They are certainly used, I'm not sure what you're arguing here. That they
should
> be able to be used inside an if()?

That you're not going to be able to just get rid of them. So there are really
two options:

(1) Make the macro more complicated so that it works as expected
(2) Leave it as is, and deal with the inevitable bugs

In my case, the bugs cause my tests to fail, leading me to detect the error. It
took me multiple hours to diagnose, because I assumed as a baseline that a macro
from base/logging.h would have the expected semantics.

My personal opinion is that (2) is a bad decision. Macros are complicated.
There's a reason their use is generally discouraged. If we choose to use them, I
think we must accept that the definitions of the macros will be complicated in
order to make their use by clients simple and reliable.

[erikwright (departed), 2015-07-09 12:45:14]

Here, incidentally, is the code snippet that caused me trouble.

It occurs in a multiprocess unit test. The child process makes some
verifications using CHECK etc. and the parent process verifies the exit code of
the child process:

      if (cmd_line->HasSwitch(kExpectGlobalSwitch))
        CHECK_NE(std::string::npos, dump.find(kGlobalString));
      else
        CHECK_EQ(std::string::npos, dump.find(kGlobalString));

While I agree that there are other ways to write this, there is nothing wrong
(on the surface) with this formulation. I don't see why we would want to leave
this trap lying hidden for other developers to step into.

[erikwright (departed), 2015-07-13 13:16:09]

On 2015/07/09 12:45:14, erikwright wrote:
> Here, incidentally, is the code snippet that caused me trouble.
> 
> It occurs in a multiprocess unit test. The child process makes some
> verifications using CHECK etc. and the parent process verifies the exit code
of
> the child process:
> 
>       if (cmd_line->HasSwitch(kExpectGlobalSwitch))
>         CHECK_NE(std::string::npos, dump.find(kGlobalString));
>       else
>         CHECK_EQ(std::string::npos, dump.find(kGlobalString));
> 
> While I agree that there are other ways to write this, there is nothing wrong
> (on the surface) with this formulation. I don't see why we would want to leave
> this trap lying hidden for other developers to step into.

Nico, Dana,

Should I conclude that you are unpersuaded and that you prefer to have a simpler
macro definition instead of the less surprising behaviour?

-Erik

[Sigurður Ásgeirsson, 2015-07-13 13:52:05]

IMHO it's a gross violation of the principle of least surprise to have the
macros behave this way. If this can be fixed, then it should be fixed,
otherwise perhaps there needs to be a clang plugin that detects this.


On Mon, Jul 13, 2015 at 1:16 PM <erikwright@chromium.org> wrote:

> On 2015/07/09 12:45:14, erikwright wrote:
> > Here, incidentally, is the code snippet that caused me trouble.
>
> > It occurs in a multiprocess unit test. The child process makes some
> > verifications using CHECK etc. and the parent process verifies the exit
> > code
> of
> > the child process:
>
> >        if (cmd_line->HasSwitch(kExpectGlobalSwitch))
> >          CHECK_NE(std::string::npos, dump.find(kGlobalString));
> >        else
> >          CHECK_EQ(std::string::npos, dump.find(kGlobalString));
>
> > While I agree that there are other ways to write this, there is nothing
> > wrong
> > (on the surface) with this formulation. I don't see why we would want to
> > leave
> > this trap lying hidden for other developers to step into.
>
> Nico, Dana,
>
> Should I conclude that you are unpersuaded and that you prefer to have a
> simpler
> macro definition instead of the less surprising behaviour?
>
> -Erik
>
> https://codereview.chromium.org/1193873004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-07-13 21:21:16]

On 2015/07/09 12:45:14, erikwright wrote:
> Here, incidentally, is the code snippet that caused me trouble.
> 
> It occurs in a multiprocess unit test. The child process makes some
> verifications using CHECK etc. and the parent process verifies the exit code
of
> the child process:
> 
>       if (cmd_line->HasSwitch(kExpectGlobalSwitch))
>         CHECK_NE(std::string::npos, dump.find(kGlobalString));
>       else
>         CHECK_EQ(std::string::npos, dump.find(kGlobalString));
> 
> While I agree that there are other ways to write this, there is nothing wrong
> (on the surface) with this formulation. I don't see why we would want to leave
> this trap lying hidden for other developers to step into.

I didn't realize that this caused you debugging work. Didn't you say above that
this is caught at compile time (with the diag being "add {}")? If it's a
compile-time error that tells you what's up I think it's ok to keep it as-is. If
it's silently compiled to the wrong thing, we should definitely do something
here.

[Sigurður Ásgeirsson, 2015-07-14 09:57:07]

The underlying macro is

  if (DCHECK_IS_ON())                                                     \
    if (std::string* _result = logging::Check##name##Impl(                \
            (val1), (val2), #val1 " " #op " " #val2))                     \
  logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result) \
      .stream()

this leads to a classic dangling else problem, as this will tuck up up to
two else statements. E.g.

if (somecond)
  DCHECK(an invariant) << "message";
else
  dowork();

turns into

if (somecond)
  if (DCHECK_IS_ON())
    if (std::string *_result)
      logging::LogMessage(...) << "message";
    else
      dowork();

which IMHO is doubleplusungood, as dowork() will now run on (somecond &&
someothercond) and not on (!somecond).

Erik, am I reading this right?

On Mon, Jul 13, 2015 at 9:21 PM <thakis@chromium.org> wrote:

> On 2015/07/09 12:45:14, erikwright wrote:
> > Here, incidentally, is the code snippet that caused me trouble.
>
> > It occurs in a multiprocess unit test. The child process makes some
> > verifications using CHECK etc. and the parent process verifies the exit
> > code
> of
> > the child process:
>
> >        if (cmd_line->HasSwitch(kExpectGlobalSwitch))
> >          CHECK_NE(std::string::npos, dump.find(kGlobalString));
> >        else
> >          CHECK_EQ(std::string::npos, dump.find(kGlobalString));
>
> > While I agree that there are other ways to write this, there is nothing
> > wrong
> > (on the surface) with this formulation. I don't see why we would want to
> > leave
> > this trap lying hidden for other developers to step into.
>
> I didn't realize that this caused you debugging work. Didn't you say above
> that
> this is caught at compile time (with the diag being "add {}")? If it's a
> compile-time error that tells you what's up I think it's ok to keep it
> as-is. If
> it's silently compiled to the wrong thing, we should definitely do
> something
> here.
>
> https://codereview.chromium.org/1193873004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2015-07-14 19:42:43]

On 2015/07/14 09:57:07, Sigurður Ásgeirsson wrote:
> The underlying macro is
> 
>   if (DCHECK_IS_ON())                                                     \
>     if (std::string* _result = logging::Check##name##Impl(                \
>             (val1), (val2), #val1 " " #op " " #val2))                     \
>   logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result) \
>       .stream()
> 
> this leads to a classic dangling else problem, as this will tuck up up to
> two else statements. E.g.
> 
> if (somecond)
>   DCHECK(an invariant) << "message";
> else
>   dowork();
> 
> turns into
> 
> if (somecond)
>   if (DCHECK_IS_ON())
>     if (std::string *_result)
>       logging::LogMessage(...) << "message";
>     else
>       dowork();
> 
> which IMHO is doubleplusungood, as dowork() will now run on (somecond &&
> someothercond) and not on (!somecond).
> 
> Erik, am I reading this right?

That's correct. This is what the change fixes.

If you fix it the naive way (without the 'switch' statement) it fixes the
problem but then you get compiler failures (on some compilers) with

if (somecond)
  DCHECK_EQ(an invariant) << "message";

Adding the 'switch' statement makes this family of statements work correctly in
all of the tested syntaxes on all of the compilers.

> 
> On Mon, Jul 13, 2015 at 9:21 PM <mailto:thakis@chromium.org> wrote:
> 
> > On 2015/07/09 12:45:14, erikwright wrote:
> > > Here, incidentally, is the code snippet that caused me trouble.
> >
> > > It occurs in a multiprocess unit test. The child process makes some
> > > verifications using CHECK etc. and the parent process verifies the exit
> > > code
> > of
> > > the child process:
> >
> > >        if (cmd_line->HasSwitch(kExpectGlobalSwitch))
> > >          CHECK_NE(std::string::npos, dump.find(kGlobalString));
> > >        else
> > >          CHECK_EQ(std::string::npos, dump.find(kGlobalString));
> >
> > > While I agree that there are other ways to write this, there is nothing
> > > wrong
> > > (on the surface) with this formulation. I don't see why we would want to
> > > leave
> > > this trap lying hidden for other developers to step into.
> >
> > I didn't realize that this caused you debugging work. Didn't you say above
> > that
> > this is caught at compile time (with the diag being "add {}")? If it's a
> > compile-time error that tells you what's up I think it's ok to keep it
> > as-is. If
> > it's silently compiled to the wrong thing, we should definitely do
> > something
> > here.
> >
> > https://codereview.chromium.org/1193873004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-07-14 19:48:44]

On 2015/07/14 09:57:07, Sigurður Ásgeirsson wrote:
> The underlying macro is
> 
>   if (DCHECK_IS_ON())                                                     \
>     if (std::string* _result = logging::Check##name##Impl(                \
>             (val1), (val2), #val1 " " #op " " #val2))                     \
>   logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result) \
>       .stream()
> 
> this leads to a classic dangling else problem, as this will tuck up up to
> two else statements. E.g.

Yes, I understand that (see above). But compilers warn on this:

Nicos-MacBook-Pro-2:clang thakis$ cat test.cc
void f() {
  if (1) if (2) ; else ;
}
Nicos-MacBook-Pro-2:clang thakis$ clang -c test.cc
test.cc:2:19: warning: add explicit braces to avoid dangling else
[-Wdangling-else]
  if (1) if (2) ; else ;
                  ^
1 warning generated.


So that's a compile-time problem (like erik said above), no?

[erikwright (departed), 2015-07-14 20:08:14]

On 2015/07/14 19:48:44, Nico wrote:
> On 2015/07/14 09:57:07, Sigurður Ásgeirsson wrote:
> > The underlying macro is
> > 
> >   if (DCHECK_IS_ON())                                                     \
> >     if (std::string* _result = logging::Check##name##Impl(                \
> >             (val1), (val2), #val1 " " #op " " #val2))                     \
> >   logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result) \
> >       .stream()
> > 
> > this leads to a classic dangling else problem, as this will tuck up up to
> > two else statements. E.g.
> 
> Yes, I understand that (see above). But compilers warn on this:
> 
> Nicos-MacBook-Pro-2:clang thakis$ cat test.cc
> void f() {
>   if (1) if (2) ; else ;
> }
> Nicos-MacBook-Pro-2:clang thakis$ clang -c test.cc
> test.cc:2:19: warning: add explicit braces to avoid dangling else
> [-Wdangling-else]
>   if (1) if (2) ; else ;
>                   ^
> 1 warning generated.
> 
> 
> So that's a compile-time problem (like erik said above), no?

It depends on your compiler.

In my case, no. The code in question was Windows-only. Even if the code was not
Windows-only, a Windows-based developer would encounter very surprising and
confusing behaviour during local development, long before submitting try jobs
and getting this compile error.

Furthermore:
(1) the compiler error is mysterious with a non-obvious cause or solution
(2) it's surprising that 'if(a) DCHECK_EQ(...)' works but 'if(a) stmt; else
DCHECK_EQ(...)' doesn't.

For further reference, here are a sampling of cases where the (D)CHECK_OP is
used in an 'if' without braces.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/sql/connection.cc&...
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol...
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert_net/nss_o...
https://code.google.com/p/chromium/codesearch#chromium/src/base/posix/unix_do...

These are the cases that are broken if you fix the 'else' case but don't use the
'switch' statement.

[Nico, 2015-07-14 20:22:42]

On 2015/07/14 20:08:14, erikwright wrote:
> On 2015/07/14 19:48:44, Nico wrote:
> > On 2015/07/14 09:57:07, Sigurður Ásgeirsson wrote:
> > > The underlying macro is
> > > 
> > >   if (DCHECK_IS_ON())                                                    
\
> > >     if (std::string* _result = logging::Check##name##Impl(               
\
> > >             (val1), (val2), #val1 " " #op " " #val2))                    
\
> > >   logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result)
\
> > >       .stream()
> > > 
> > > this leads to a classic dangling else problem, as this will tuck up up to
> > > two else statements. E.g.
> > 
> > Yes, I understand that (see above). But compilers warn on this:
> > 
> > Nicos-MacBook-Pro-2:clang thakis$ cat test.cc
> > void f() {
> >   if (1) if (2) ; else ;
> > }
> > Nicos-MacBook-Pro-2:clang thakis$ clang -c test.cc
> > test.cc:2:19: warning: add explicit braces to avoid dangling else
> > [-Wdangling-else]
> >   if (1) if (2) ; else ;
> >                   ^
> > 1 warning generated.
> > 
> > 
> > So that's a compile-time problem (like erik said above), no?
> 
> It depends on your compiler.
> 
> In my case, no. The code in question was Windows-only. Even if the code was
not
> Windows-only, a Windows-based developer would encounter very surprising and
> confusing behaviour during local development, long before submitting try jobs
> and getting this compile error.

Windows developers have compiler choices too :-P

But ok, that's a good point, I didn't realize that cl silently does the wrong
thing here.

> Furthermore:
> (1) the compiler error is mysterious with a non-obvious cause or solution

It says "add braces", that doesn't seem terribly mysterious to me.

> (2) it's surprising that 'if(a) DCHECK_EQ(...)' works but 'if(a) stmt; else
> DCHECK_EQ(...)' doesn't.

Yeah, but we got by without this for years. So it's not super like this is super
important in practice.

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h#newcode511
base/logging.h:511: logging::LogMessage(__FILE__, __LINE__,
_result.message()).stream()
So if you say

  DCHECK() << "foo";

won't the << now bind to the logging::LogMessage() in the else branch? I.e. the
message is logged if the dcheck passes, but not if it fails?

[erikwright (departed), 2015-07-14 20:33:00]

On 2015/07/14 20:22:42, Nico wrote:
> On 2015/07/14 20:08:14, erikwright wrote:
> > On 2015/07/14 19:48:44, Nico wrote:
> > > On 2015/07/14 09:57:07, Sigurður Ásgeirsson wrote:
> > > > The underlying macro is
> > > > 
> > > >   if (DCHECK_IS_ON())                                                   

> \
> > > >     if (std::string* _result = logging::Check##name##Impl(              

> \
> > > >             (val1), (val2), #val1 " " #op " " #val2))                   

> \
> > > >   logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK,
_result)
> \
> > > >       .stream()
> > > > 
> > > > this leads to a classic dangling else problem, as this will tuck up up
to
> > > > two else statements. E.g.
> > > 
> > > Yes, I understand that (see above). But compilers warn on this:
> > > 
> > > Nicos-MacBook-Pro-2:clang thakis$ cat test.cc
> > > void f() {
> > >   if (1) if (2) ; else ;
> > > }
> > > Nicos-MacBook-Pro-2:clang thakis$ clang -c test.cc
> > > test.cc:2:19: warning: add explicit braces to avoid dangling else
> > > [-Wdangling-else]
> > >   if (1) if (2) ; else ;
> > >                   ^
> > > 1 warning generated.
> > > 
> > > 
> > > So that's a compile-time problem (like erik said above), no?
> > 
> > It depends on your compiler.
> > 
> > In my case, no. The code in question was Windows-only. Even if the code was
> not
> > Windows-only, a Windows-based developer would encounter very surprising and
> > confusing behaviour during local development, long before submitting try
jobs
> > and getting this compile error.
> 
> Windows developers have compiler choices too :-P
> 
> But ok, that's a good point, I didn't realize that cl silently does the wrong
> thing here.
> 
> > Furthermore:
> > (1) the compiler error is mysterious with a non-obvious cause or solution
> 
> It says "add braces", that doesn't seem terribly mysterious to me.
> 
> > (2) it's surprising that 'if(a) DCHECK_EQ(...)' works but 'if(a) stmt; else
> > DCHECK_EQ(...)' doesn't.
> 
> Yeah, but we got by without this for years. So it's not super like this is
super
> important in practice.

With all humility, I think that I have an above-average tenacity for fixing
problems like this.

It is entirely possible that someone else spent hours debugging this, realized
it was broken, and just changed their code, leaving the mess for someone else to
step in after them.

Since it has been hit at least once (by me) it seems likely that it has
been/will be hit again by someone else.

> https://codereview.chromium.org/1193873004/diff/120001/base/logging.h
> File base/logging.h (right):
> 
>
https://codereview.chromium.org/1193873004/diff/120001/base/logging.h#newcode511
> base/logging.h:511: logging::LogMessage(__FILE__, __LINE__,
> _result.message()).stream()
> So if you say
> 
>   DCHECK() << "foo";
> 
> won't the << now bind to the logging::LogMessage() in the else branch? I.e.
the
> message is logged if the dcheck passes, but not if it fails?

[erikwright (departed), 2015-07-14 20:33:08]

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h#newcode511
base/logging.h:511: logging::LogMessage(__FILE__, __LINE__,
_result.message()).stream()
On 2015/07/14 20:22:41, Nico wrote:
> So if you say
> 
>   DCHECK() << "foo";
> 
> won't the << now bind to the logging::LogMessage() in the else branch? I.e.
the
> message is logged if the dcheck passes, but not if it fails?

It binds to the else, as desired. Note that CheckOpResult::operator bool()
returns true if the check _succeeds_. So the 'else' case is what is hit when the
check fails.

[Nico, 2015-07-14 20:38:57]

> It is entirely possible that someone else spent hours debugging this, realized
> it was broken, and just changed their code, leaving the mess for someone else
to
> step in after them.

(This isn't a problem in any checked-in code, else bots would be red.)

[Nico, 2015-07-14 20:47:10]

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1193873004/diff/120001/base/logging.h#newcode511
base/logging.h:511: logging::LogMessage(__FILE__, __LINE__,
_result.message()).stream()
On 2015/07/14 20:33:08, erikwright wrote:
> On 2015/07/14 20:22:41, Nico wrote:
> > So if you say
> > 
> >   DCHECK() << "foo";
> > 
> > won't the << now bind to the logging::LogMessage() in the else branch? I.e.
> the
> > message is logged if the dcheck passes, but not if it fails?
> 
> It binds to the else, as desired. Note that CheckOpResult::operator bool()
> returns true if the check _succeeds_. So the 'else' case is what is hit when
the
> check fails.

Ah, sorry. This might be too tricky for people with below-average code reading
abilities like me :-)

Ok, lgtm, but remove the leading _ on result (and rename it to something else if
this causes problems) – names starting with underscores are reserved for the
implementation.

[erikwright (departed), 2015-07-15 18:32:27]

siggi: PTAL at the updated tests.

[Sigurður Ásgeirsson, 2015-07-16 10:51:28]

lgtm - the repetition is a PITA, but I don't think there's any reasonable way
around it...

https://codereview.chromium.org/1193873004/diff/200001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/1193873004/diff/200001/base/logging_unittest....
base/logging_unittest.cc:306: EvaluatedOnceIfDCheckIsOn bool_expr;
nit: IMHO the use of this would be marginally more readable if this were named
eval_once or some such. No biggie.

[erikwright (departed), 2015-07-16 14:39:56]

On 2015/07/16 10:51:28, Sigurður Ásgeirsson wrote:
> lgtm - the repetition is a PITA, but I don't think there's any reasonable way
> around it...
> 
>
https://codereview.chromium.org/1193873004/diff/200001/base/logging_unittest.cc
> File base/logging_unittest.cc (right):
> 
>
https://codereview.chromium.org/1193873004/diff/200001/base/logging_unittest....
> base/logging_unittest.cc:306: EvaluatedOnceIfDCheckIsOn bool_expr;
> nit: IMHO the use of this would be marginally more readable if this were named
> eval_once or some such. No biggie.

I uploaded a new version that uses a macro to define the common test structure.

On the one hand, it seems horrific, on the other hand it eliminates the risk of
a copy-paste error, makes it easier to add test cases, should presumably be
easier to properly review.

WDYT?

[erikwright (departed), 2015-07-16 20:14:14]

The CQ bit was checked by erikwright@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-16 20:15:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1193873004/220001

[Nico, 2015-07-16 20:25:22]

I don't think this needs exhaustive testing. Add a test for one or two cases you
change behavior for and be done with it.

[erikwright (departed), 2015-07-16 20:41:18]

On 2015/07/16 20:25:22, Nico wrote:
> I don't think this needs exhaustive testing. Add a test for one or two cases
you
> change behavior for and be done with it.

I wanted at a minimum to cover switch and if/else for both DCHECK_OP and
CHECK_OP.

So that's pretty much everything that is in the test case. If I only do those
two, I could duplicate the test case (as it was in patchset 11) or deduplicate
it as in patchset 12.

If I'm going to de-dup, it seems silly to not benefit from the opportunity to
protect (D)LOG_IF and (D)CHECK from potential regression, for virtually free.

But I'll do whatever you want. Do I test two macros with identical and
duplicated test cases? Two macros with the deduplicated test cases? Or 6 macros
with the deduplicated test cases?

[Nico, 2015-07-16 21:02:03]

I don't care too much, but it shouldn't be more than around 25 lines. The whole
test file is currently ~300 lines and tripling this for this corner case bug
seems excessive. Have one if/else with a DCHECK and one switch with a CHECK I
guess.

[commit-bot: I haz the power, 2015-07-16 21:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-16 21:39:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[erikwright (departed), 2015-07-17 16:04:12]

Patchset #13 (id:240001) has been deleted

[erikwright (departed), 2015-07-17 17:21:26]

Patchset #13 (id:260001) has been deleted

[erikwright (departed), 2015-07-17 17:22:08]

Patchset #13 (id:280001) has been deleted

[erikwright (departed), 2015-07-17 17:23:03]

Patchset #13 (id:300001) has been deleted

[erikwright (departed), 2015-07-17 17:25:20]

The test suite was originally 258 lines. In my personal opinion it is
incomplete.

Patchset 14 contains 24 lines of additional test coverage; the bare minimum that
is useful to cover the (D)CHECK_OP regression fixed in this CL.

Patchset 13, adds 174 lines of additional test coverage; this is significant
coverage that was previously lacking for six different macros.

The new coverage verifies that the statements behave correctly in both branches
of an if/else, in a standalone if, and in a switch statement.

The coverage additionally verifies that the conditional expressions are only
evaluated if the check is enabled and reached.

I gather from your prior comment that patchset 14 is what you had in mind, so
PTAL at it.

[Nico, 2015-07-17 20:29:35]

lgtm

[erikwright (departed), 2015-07-22 18:24:33]

The CQ bit was checked by erikwright@chromium.org

[erikwright (departed), 2015-07-22 18:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/1193873004/#ps340001
(title: "Cut back test coverage.")

[commit-bot: I haz the power, 2015-07-22 18:25:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1193873004/340001

[commit-bot: I haz the power, 2015-07-22 20:06:00]

Committed patchset #14 (id:340001)

[commit-bot: I haz the power, 2015-07-22 20:07:37]

Patchset 14 (id:??) landed as
https://crrev.com/6ad937bc698ce8958d6578609919fe9fd05429f6
Cr-Commit-Position: refs/heads/master@{#339943}