NOTREACHED should use DCHECK if DCHECK_ALWAYS_ON is set on cros.

NOTREACHED should use DCHECK if DCHECK_ALWAYS_ON is set on cros.

Committed: https://crrev.com/e8ed3db4cc220eacaa065d32cbd8f1f1ca8a3a91
Cr-Commit-Position: refs/heads/master@{#301555}

[oshima, 2014-10-27 21:32:10]

oshima@chromium.org changed reviewers:
+ thestig@chromium.org

[Peter Kasting, 2014-10-27 21:36:18]

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

[Peter Kasting, 2014-10-27 21:36:18]

https://codereview.chromium.org/686573003/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
This seems fragile.  We're slowly redeclaring all the conditions that control
DCHECK.

Can we instead do something like:

#if defined(NDEBUG) && defined(OS_CHROMEOS)
#define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while (false)
#else
#define NOTREACHED() DCHECK(false)
#endif

Or maybe even better, make DCHECK failure in ChromeOS release builds be
LOG(ERROR) instead of nothing, eliminating the need to define something here,
and catching even more cases?

[scottmg, 2014-10-27 21:38:04]

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

[scottmg, 2014-10-27 21:38:04]

https://codereview.chromium.org/686573003/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:36:18, Peter Kasting wrote:
> This seems fragile.  We're slowly redeclaring all the conditions that control
> DCHECK.
> 
> Can we instead do something like:
> 
> #if defined(NDEBUG) && defined(OS_CHROMEOS)
> #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while (false)
> #else
> #define NOTREACHED() DCHECK(false)
> #endif
> 
> Or maybe even better, make DCHECK failure in ChromeOS release builds be
> LOG(ERROR) instead of nothing, eliminating the need to define something here,
> and catching even more cases?

(do while(0) won't work with notreached() << "stuff";)

[dmichael (off chromium), 2014-10-27 21:40:25]

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

[dmichael (off chromium), 2014-10-27 21:40:25]

https://codereview.chromium.org/686573003/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:38:04, scottmg wrote:
> On 2014/10/27 21:36:18, Peter Kasting wrote:
> > This seems fragile.  We're slowly redeclaring all the conditions that
control
> > DCHECK.
> > 
> > Can we instead do something like:
> > 
> > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
(false)
> > #else
> > #define NOTREACHED() DCHECK(false)
> > #endif
> > 
> > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > LOG(ERROR) instead of nothing, eliminating the need to define something
here,
> > and catching even more cases?
> 
> (do while(0) won't work with notreached() << "stuff";)
do while won't, but it might be OK to use:
DCHECK(false) << "NOTREACHED() hit in..."
so that NOTREACHED will expand to just another <<. Then you could still not have
to rely on DCHECK_ALWAYS_ON

[danakj, 2014-10-27 21:42:34]

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

[danakj, 2014-10-27 21:42:34]

https://codereview.chromium.org/686573003/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:38:04, scottmg wrote:
> On 2014/10/27 21:36:18, Peter Kasting wrote:
> > This seems fragile.  We're slowly redeclaring all the conditions that
control
> > DCHECK.
> > 
> > Can we instead do something like:
> > 
> > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
(false)
> > #else
> > #define NOTREACHED() DCHECK(false)
> > #endif
> > 
> > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > LOG(ERROR) instead of nothing, eliminating the need to define something
here,
> > and catching even more cases?
> 
> (do while(0) won't work with notreached() << "stuff";)

Why not #if DCHECK_IS_ON?

[oshima, 2014-10-27 22:04:16]

oshima@chromium.org changed reviewers:
+ zelidrag@zelidrag.com, zork@chromium.org

[oshima, 2014-10-27 22:04:17]

https://codereview.chromium.org/686573003/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:42:34, danakj wrote:
> On 2014/10/27 21:38:04, scottmg wrote:
> > On 2014/10/27 21:36:18, Peter Kasting wrote:
> > > This seems fragile.  We're slowly redeclaring all the conditions that
> control
> > > DCHECK.
> > > 
> > > Can we instead do something like:
> > > 
> > > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
> (false)
> > > #else
> > > #define NOTREACHED() DCHECK(false)
> > > #endif
> > > 
> > > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > > LOG(ERROR) instead of nothing, eliminating the need to define something
> here,
> > > and catching even more cases?
> > 
> > (do while(0) won't work with notreached() << "stuff";)
> 
> Why not #if DCHECK_IS_ON?

Sure done.

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:40:25, dmichael wrote:
> On 2014/10/27 21:38:04, scottmg wrote:
> > On 2014/10/27 21:36:18, Peter Kasting wrote:
> > > This seems fragile.  We're slowly redeclaring all the conditions that
> control
> > > DCHECK.
> > > 
> > > Can we instead do something like:
> > > 
> > > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
> (false)
> > > #else
> > > #define NOTREACHED() DCHECK(false)
> > > #endif
> > > 
> > > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > > LOG(ERROR) instead of nothing, eliminating the need to define something
> here,
> > > and catching even more cases?
> > 
> > (do while(0) won't work with notreached() << "stuff";)
> do while won't, but it might be OK to use:
> DCHECK(false) << "NOTREACHED() hit in..."
> so that NOTREACHED will expand to just another <<. Then you could still not
have
> to rely on DCHECK_ALWAYS_ON

IIRC, DCHECK is no-op on normal rel build.

https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
defined(OS_CHROMEOS)
On 2014/10/27 21:36:18, Peter Kasting wrote:
> This seems fragile.  We're slowly redeclaring all the conditions that control
> DCHECK.
> 
> Can we instead do something like:
> 
> #if defined(NDEBUG) && defined(OS_CHROMEOS)
> #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while (false)
> #else
> #define NOTREACHED() DCHECK(false)
> #endif
> 
> Or maybe even better, make DCHECK failure in ChromeOS release builds be
> LOG(ERROR) instead of nothing, eliminating the need to define something here,
> and catching even more cases?

looking at the bug crbug.com/288014, I wonder if we want to keep this.
zork@, zel@, you guys added this line. What do you want to keep this or ok to
remove?

[oshima, 2014-10-27 22:06:14]

Patchset #2 (id:20001) has been deleted

[oshima, 2014-10-28 00:33:14]

On 2014/10/27 22:04:17, oshima wrote:
> https://codereview.chromium.org/686573003/diff/1/base/logging.h
> File base/logging.h (right):
> 
> https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
> base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
> defined(OS_CHROMEOS)
> On 2014/10/27 21:42:34, danakj wrote:
> > On 2014/10/27 21:38:04, scottmg wrote:
> > > On 2014/10/27 21:36:18, Peter Kasting wrote:
> > > > This seems fragile.  We're slowly redeclaring all the conditions that
> > control
> > > > DCHECK.
> > > > 
> > > > Can we instead do something like:
> > > > 
> > > > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > > > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
> > (false)
> > > > #else
> > > > #define NOTREACHED() DCHECK(false)
> > > > #endif
> > > > 
> > > > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > > > LOG(ERROR) instead of nothing, eliminating the need to define something
> > here,
> > > > and catching even more cases?
> > > 
> > > (do while(0) won't work with notreached() << "stuff";)
> > 
> > Why not #if DCHECK_IS_ON?
> 
> Sure done.
> 
> https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
> base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
> defined(OS_CHROMEOS)
> On 2014/10/27 21:40:25, dmichael wrote:
> > On 2014/10/27 21:38:04, scottmg wrote:
> > > On 2014/10/27 21:36:18, Peter Kasting wrote:
> > > > This seems fragile.  We're slowly redeclaring all the conditions that
> > control
> > > > DCHECK.
> > > > 
> > > > Can we instead do something like:
> > > > 
> > > > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > > > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
> > (false)
> > > > #else
> > > > #define NOTREACHED() DCHECK(false)
> > > > #endif
> > > > 
> > > > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > > > LOG(ERROR) instead of nothing, eliminating the need to define something
> > here,
> > > > and catching even more cases?
> > > 
> > > (do while(0) won't work with notreached() << "stuff";)
> > do while won't, but it might be OK to use:
> > DCHECK(false) << "NOTREACHED() hit in..."
> > so that NOTREACHED will expand to just another <<. Then you could still not
> have
> > to rely on DCHECK_ALWAYS_ON
> 
> IIRC, DCHECK is no-op on normal rel build.
> 
> https://codereview.chromium.org/686573003/diff/1/base/logging.h#newcode704
> base/logging.h:704: #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON) &&
> defined(OS_CHROMEOS)
> On 2014/10/27 21:36:18, Peter Kasting wrote:
> > This seems fragile.  We're slowly redeclaring all the conditions that
control
> > DCHECK.
> > 
> > Can we instead do something like:
> > 
> > #if defined(NDEBUG) && defined(OS_CHROMEOS)
> > #define NOTREACHED() do { LOG(ERROR) << "blah"; DCHECK(false); } while
(false)
> > #else
> > #define NOTREACHED() DCHECK(false)
> > #endif
> > 
> > Or maybe even better, make DCHECK failure in ChromeOS release builds be
> > LOG(ERROR) instead of nothing, eliminating the need to define something
here,
> > and catching even more cases?
> 
> looking at the bug crbug.com/288014, I wonder if we want to keep this.
> zork@, zel@, you guys added this line. What do you want to keep this or ok to
> remove?

zel@ wants to keep it. danajk@, PTAL

[danakj, 2014-10-28 00:35:02]

This seems really bizarre, to only kill 1 type of asserts... I had no idea this
was here and I don't agree with this existing at all without any context on why
it's here.

But anyway, LGTM as this is definitely better than what's there today.

[danakj, 2014-10-28 00:36:07]

On 2014/10/28 00:35:02, danakj wrote:
> This seems really bizarre, to only kill 1 type of asserts... I had no idea
this
> was here and I don't agree with this existing at all without any context on
why
> it's here.
> 
> But anyway, LGTM as this is definitely better than what's there today.

Oh, I guess this is just making it verbose when it's turned off? Still weird,
less controversial maybe. :)

[oshima, 2014-10-28 00:53:03]

On 2014/10/28 00:36:07, danakj wrote:
> On 2014/10/28 00:35:02, danakj wrote:
> > This seems really bizarre, to only kill 1 type of asserts... I had no idea
> this
> > was here and I don't agree with this existing at all without any context on
> why
> > it's here.
> > 
> > But anyway, LGTM as this is definitely better than what's there today.
> 
> Oh, I guess this is just making it verbose when it's turned off? Still weird,
> less controversial maybe. :)

Yeah, it may look weird, but this is to record this event on a device so this is
intentional.

[oshima, 2014-10-28 00:53:18]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-10-28 00:53:30]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-10-28 00:53:45]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-10-28 00:55:48]

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

[commit-bot: I haz the power, 2014-10-28 02:32:21]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2014-10-28 02:33:12]

Patchset 2 (id:??) landed as
https://crrev.com/e8ed3db4cc220eacaa065d32cbd8f1f1ca8a3a91
Cr-Commit-Position: refs/heads/master@{#301555}