Fixed subtle difference in behavior between DCHECK and DCHECK_EQ et al.

Fixed subtle difference in behavior between DCHECK and DCHECK_EQ et al.

BUG=58998
TEST=New unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64650

[akalin, 2010-10-29 00:32:01]

+wtc for review

[wtc, 2010-10-29 20:55:35]

brettw: please review this CL as the owner of base/logging.h.

akalin: thanks for writing the CL.  I did my best to review
this CL, and it looks good to me except for the issue marked
with "IMPORTANT" below.  All my other comments are just nits.

http://codereview.chromium.org/4199005/diff/1/2
File base/logging.h (right):

http://codereview.chromium.org/4199005/diff/1/2#newcode528
base/logging.h:528: #define DLOG_IS_ON(severity) (LOG_IS_ON(severity))
Nit: do we need the parentheses around LOG_IS_ON(severity)?

LOG_IS_ON is defined as:

#define LOG_IS_ON(severity) \
  ((::logging::LOG_ ## severity) >= ::logging::GetMinLogLevel())

So its expansion is already parenthesized.

This comment also applies to the DCHECK_IS_ON definition on
line 600.

http://codereview.chromium.org/4199005/diff/1/2#newcode544
base/logging.h:544: #define DLOG_IS_ON(severity) (false)
Nit: 'false' is a single word, so we should not need to add ()
around it.

This comment also applies to the DCHECK_IS_ON definition on
line 608.

http://codereview.chromium.org/4199005/diff/1/2#newcode600
base/logging.h:600: #define DCHECK_IS_ON() (LOG_IS_ON(DCHECK))
IMPORTANT: is this change correct?  Could you explain why
we don't define DCHECK_IS_ON() as 'true' in this case?

Note: the correctness of this change is related to the
correctness of the change on line 593 above.

If this change is correct, do we need to update the comment
on line 597?

http://codereview.chromium.org/4199005/diff/1/3
File base/logging_unittest.cc (right):

http://codereview.chromium.org/4199005/diff/1/3#newcode166
base/logging_unittest.cc:166: #else  // defined(LOGGING_IS_OFFICIAL_BUILD)
Nit: this comment is confusing because the immediate ifdef
tests NDEBUG as opposed to LOGGING_IS_OFFICIAL_BUILD.

[akalin, 2010-11-01 17:55:42]

http://codereview.chromium.org/4199005/diff/1/2
File base/logging.h (right):

http://codereview.chromium.org/4199005/diff/1/2#newcode528
base/logging.h:528: #define DLOG_IS_ON(severity) (LOG_IS_ON(severity))
On 2010/10/29 20:55:35, wtc wrote:
> Nit: do we need the parentheses around LOG_IS_ON(severity)?
> 
> LOG_IS_ON is defined as:
> 
> #define LOG_IS_ON(severity) \
>   ((::logging::LOG_ ## severity) >= ::logging::GetMinLogLevel())
> 
> So its expansion is already parenthesized.
> 
> This comment also applies to the DCHECK_IS_ON definition on
> line 600.

Done.

http://codereview.chromium.org/4199005/diff/1/2#newcode544
base/logging.h:544: #define DLOG_IS_ON(severity) (false)
On 2010/10/29 20:55:35, wtc wrote:
> Nit: 'false' is a single word, so we should not need to add ()
> around it.
> 
> This comment also applies to the DCHECK_IS_ON definition on
> line 608.

Done.

http://codereview.chromium.org/4199005/diff/1/2#newcode600
base/logging.h:600: #define DCHECK_IS_ON() (LOG_IS_ON(DCHECK))
On 2010/10/29 20:55:35, wtc wrote:
> IMPORTANT: is this change correct?  Could you explain why
> we don't define DCHECK_IS_ON() as 'true' in this case?
> 
> Note: the correctness of this change is related to the
> correctness of the change on line 593 above.
> 
> If this change is correct, do we need to update the comment
> on line 597?

Basically, even if we defined DCHECK_IS_ON() as true here, a user could just set
the log level to be >FATAL and cause DCHECKs not to fire.  This is a problem
I've been meaning to fix, but I have this here to keep the lazy behavior of
DCHECK consistent.

Added TODO explaining the situation.  I don't think the comment needs updating,
as that's what we eventually want to happen. :)

http://codereview.chromium.org/4199005/diff/1/3
File base/logging_unittest.cc (right):

http://codereview.chromium.org/4199005/diff/1/3#newcode166
base/logging_unittest.cc:166: #else  // defined(LOGGING_IS_OFFICIAL_BUILD)
On 2010/10/29 20:55:35, wtc wrote:
> Nit: this comment is confusing because the immediate ifdef
> tests NDEBUG as opposed to LOGGING_IS_OFFICIAL_BUILD.

I agree this is confusing, but I thought the style was to comment #else and
#endif with the original condition.  Removed.

[brettw, 2010-11-01 18:10:15]

What about DLOG_IF(FATAL). Is this controlled by the same thing as DCHECK in
release modes?

[wtc, 2010-11-01 18:30:46]

LGTM.  Please wait for brettw's review before checking this in.
Thanks!

[akalin, 2010-11-01 18:36:33]

On Mon, Nov 1, 2010 at 12:10 PM,  <brettw@chromium.org> wrote:
> What about DLOG_IF(FATAL). Is this controlled by the same thing as DCHECK in
> release modes?

No.  DLOGs are on only for unofficial, debug builds and have no
runtime switch for release modes.

[brettw, 2010-11-01 19:29:01]

This is confusing since DCHECK is the same as DLOG_IF(FATAL), but I guess it
doesn't need to hold up this patch. LGTM

[akalin, 2010-11-01 19:49:00]

On 2010/11/01 19:29:01, brettw wrote:
> This is confusing since DCHECK is the same as DLOG_IF(FATAL), but I guess it
> doesn't need to hold up this patch. LGTM

Yeah, I have mixed feelings about the difference in behavior, but that's always
been around.

Submitting.