Replace ASSERTs in platform/heap/ with DCHECKs

Replace ASSERTs in platform/heap/ with DCHECKs

- ASSERT => DCHECK, DCHECK_EQ, DCHECK_LT etc
- RELEASE_ASSERT => CHECK
- ENABLE(ASSERT) => DCHECK_IS_ON

BUG=596760

[haraken, 2017-01-06 04:18:44]

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

[haraken, 2017-01-06 04:18:48]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2017-01-06 04:18:49]

PTAL

https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/GCInfo.h (right):

https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GCInfo.h:165: static const size_t
gcInfoMaxIndex = 1 << 14;

I hit a link error if I put this in GCInfo. I couldn't work around it, so put it
outside the class declaration.

[commit-bot: I haz the power, 2017-01-06 04:18:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 04:54:01]

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

[commit-bot: I haz the power, 2017-01-06 04:54:02]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[haraken, 2017-01-06 05:19:08]

Hmm.

https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...

This error indicates that we have to remove '#if DCHECK_IS_ON()' from methods
used in DCHECKs? It looks stupid that we have to unconditionally declare/define
methods used only inside DCHECKs...

[sof, 2017-01-06 08:31:33]

On 2017/01/06 05:19:08, haraken wrote:
> Hmm.
> 
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...
> 
> This error indicates that we have to remove '#if DCHECK_IS_ON()' from methods
> used in DCHECKs? It looks stupid that we have to unconditionally
declare/define
> methods used only inside DCHECKs...

Likely to be some preferred idiom to avoid having to do that in code that is
already DCHECK-based? (but I don't know what&where that might be.)

Not entirely serious, but an alternative is to re-introduce ASSERT() --

#if DCHECK_IS_ON()
#define ENABLE_ASSERT 1
#define ASSERT(condition) DCHECK(condition)
#else
#define ENABLE_ASSERT 0
#define ASSERT(condition)
#endif

:)

[haraken, 2017-01-06 09:01:07]

On 2017/01/06 08:31:33, sof wrote:
> On 2017/01/06 05:19:08, haraken wrote:
> > Hmm.
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...
> > 
> > This error indicates that we have to remove '#if DCHECK_IS_ON()' from
methods
> > used in DCHECKs? It looks stupid that we have to unconditionally
> declare/define
> > methods used only inside DCHECKs...
> 
> Likely to be some preferred idiom to avoid having to do that in code that is
> already DCHECK-based? (but I don't know what&where that might be.)
> 
> Not entirely serious, but an alternative is to re-introduce ASSERT() --
> 
> #if DCHECK_IS_ON()
> #define ENABLE_ASSERT 1
> #define ASSERT(condition) DCHECK(condition)
> #else
> #define ENABLE_ASSERT 0
> #define ASSERT(condition)
> #endif
> 
> :)

At a first glance, people are adding #if DCHECK_IS_ON() on the caller side:

#if DCHECK_IS_ON()
  DCHECK(checkHeader());
#endif

which I want to avoid.

[haraken, 2017-01-06 09:08:02]

haraken@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org

[haraken, 2017-01-06 09:08:02]

+dcheng +danakj

What's an idiom to use dcheck-only methods? The following code doesn't compile.
(See the compile failure in PS3 for more details.)

#if DCHECK_IS_ON()
  bool checkHeader() { ... }
#endif

  DCHECK(checkHeader());

We can fix the compile failure by doing either:

a) Drop DCHECK_IS_ON() from the method definition; or

b) Add DCHECK_IS_ON() to DCHECK(checkHeader()).

However, a) has a risk of making the compiled code less efficient (I don't
know). b) will mess up the code base.

Any idea?

[dcheng, 2017-01-06 11:41:21]

On 2017/01/06 09:08:02, haraken wrote:
> +dcheng +danakj
> 
> What's an idiom to use dcheck-only methods? The following code doesn't
compile.
> (See the compile failure in PS3 for more details.)
> 
> #if DCHECK_IS_ON()
>   bool checkHeader() { ... }
> #endif
> 
>   DCHECK(checkHeader());
> 
> We can fix the compile failure by doing either:
> 
> a) Drop DCHECK_IS_ON() from the method definition; or
> 
> b) Add DCHECK_IS_ON() to DCHECK(checkHeader()).
> 
> However, a) has a risk of making the compiled code less efficient (I don't
> know). b) will mess up the code base.
> 
> Any idea?

In Chrome, we usually go with (a). DCHECK macro looks like this:

#define DCHECK(condition)                                                \
  LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
      << "Check failed: " #condition ". "

so if DCHECK_ON() is false, the compiler will simplify this by collapsing:
  DCHECK_IS_ON() ? !(condition) : false
to just:
  false

For the unused method, I think we usually depend on dead-code elimination to
remove it, but I can't find any good examples at the moment...

[danakj, 2017-01-06 15:17:34]

On Fri, Jan 6, 2017 at 6:41 AM, <dcheng@chromium.org> wrote:

> On 2017/01/06 09:08:02, haraken wrote:
> > +dcheng +danakj
> >
> > What's an idiom to use dcheck-only methods? The following code doesn't
> compile.
> > (See the compile failure in PS3 for more details.)
> >
> > #if DCHECK_IS_ON()
> > bool checkHeader() { ... }
> > #endif
> >
> > DCHECK(checkHeader());
> >
> > We can fix the compile failure by doing either:
> >
> > a) Drop DCHECK_IS_ON() from the method definition; or
> >
> > b) Add DCHECK_IS_ON() to DCHECK(checkHeader()).
> >
> > However, a) has a risk of making the compiled code less efficient (I
> don't
> > know). b) will mess up the code base.
> >
> > Any idea?
>
> In Chrome, we usually go with (a). DCHECK macro looks like this:
>
> #define DCHECK(condition) \
> LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
> << "Check failed: " #condition ". "
>
> so if DCHECK_ON() is false, the compiler will simplify this by collapsing:
> DCHECK_IS_ON() ? !(condition) : false
> to just:
> false
>
> For the unused method, I think we usually depend on dead-code elimination
> to
> remove it, but I can't find any good examples at the moment...
>

Yes, usually leave the method and #if DCHECK_IS_ON() the contents of its
body. Example:
https://cs.chromium.org/chromium/src/cc/tiles/picture_layer_tiling.cc?rcl=0&l...


>
> https://codereview.chromium.org/2619493003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-01-06 15:17:34]

On Fri, Jan 6, 2017 at 6:41 AM, <dcheng@chromium.org> wrote:

> On 2017/01/06 09:08:02, haraken wrote:
> > +dcheng +danakj
> >
> > What's an idiom to use dcheck-only methods? The following code doesn't
> compile.
> > (See the compile failure in PS3 for more details.)
> >
> > #if DCHECK_IS_ON()
> > bool checkHeader() { ... }
> > #endif
> >
> > DCHECK(checkHeader());
> >
> > We can fix the compile failure by doing either:
> >
> > a) Drop DCHECK_IS_ON() from the method definition; or
> >
> > b) Add DCHECK_IS_ON() to DCHECK(checkHeader()).
> >
> > However, a) has a risk of making the compiled code less efficient (I
> don't
> > know). b) will mess up the code base.
> >
> > Any idea?
>
> In Chrome, we usually go with (a). DCHECK macro looks like this:
>
> #define DCHECK(condition) \
> LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
> << "Check failed: " #condition ". "
>
> so if DCHECK_ON() is false, the compiler will simplify this by collapsing:
> DCHECK_IS_ON() ? !(condition) : false
> to just:
> false
>
> For the unused method, I think we usually depend on dead-code elimination
> to
> remove it, but I can't find any good examples at the moment...
>

Yes, usually leave the method and #if DCHECK_IS_ON() the contents of its
body. Example:
https://cs.chromium.org/chromium/src/cc/tiles/picture_layer_tiling.cc?rcl=0&l...


>
> https://codereview.chromium.org/2619493003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2017-01-06 15:18:20]

On 2017/01/06 15:17:34, danakj_OOO_and_slow wrote:
> On Fri, Jan 6, 2017 at 6:41 AM, <mailto:dcheng@chromium.org> wrote:
> 
> > On 2017/01/06 09:08:02, haraken wrote:
> > > +dcheng +danakj
> > >
> > > What's an idiom to use dcheck-only methods? The following code doesn't
> > compile.
> > > (See the compile failure in PS3 for more details.)
> > >
> > > #if DCHECK_IS_ON()
> > > bool checkHeader() { ... }
> > > #endif
> > >
> > > DCHECK(checkHeader());
> > >
> > > We can fix the compile failure by doing either:
> > >
> > > a) Drop DCHECK_IS_ON() from the method definition; or
> > >
> > > b) Add DCHECK_IS_ON() to DCHECK(checkHeader()).
> > >
> > > However, a) has a risk of making the compiled code less efficient (I
> > don't
> > > know). b) will mess up the code base.
> > >
> > > Any idea?
> >
> > In Chrome, we usually go with (a). DCHECK macro looks like this:
> >
> > #define DCHECK(condition) \
> > LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
> > << "Check failed: " #condition ". "
> >
> > so if DCHECK_ON() is false, the compiler will simplify this by collapsing:
> > DCHECK_IS_ON() ? !(condition) : false
> > to just:
> > false
> >
> > For the unused method, I think we usually depend on dead-code elimination
> > to
> > remove it, but I can't find any good examples at the moment...
> >
> 
> Yes, usually leave the method and #if DCHECK_IS_ON() the contents of its
> body. Example:
>
https://cs.chromium.org/chromium/src/cc/tiles/picture_layer_tiling.cc?rcl=0&l...

Thanks, I'll follow the convention :)

[sof, 2017-01-08 07:10:47]

https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/GCInfo.h (right):

https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GCInfo.h:165: static const size_t
gcInfoMaxIndex = 1 << 14;
On 2017/01/06 04:18:49, haraken wrote:
> 
> I hit a link error if I put this in GCInfo. I couldn't work around it, so put
it
> outside the class declaration.
> 

Do you remember what the link error was?

[haraken, 2017-01-08 13:29:02]

On 2017/01/08 07:10:47, sof wrote:
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/GCInfo.h (right):
> 
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/GCInfo.h:165: static const size_t
> gcInfoMaxIndex = 1 << 14;
> On 2017/01/06 04:18:49, haraken wrote:
> > 
> > I hit a link error if I put this in GCInfo. I couldn't work around it, so
put
> it
> > outside the class declaration.
> > 
> 
> Do you remember what the link error was?

See the error in PS3. It happens only on ChromeOS.

[sof, 2017-01-08 13:31:17]

On 2017/01/08 13:29:02, haraken wrote:
> On 2017/01/08 07:10:47, sof wrote:
> >
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/heap/GCInfo.h (right):
> > 
> >
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/heap/GCInfo.h:165: static const size_t
> > gcInfoMaxIndex = 1 << 14;
> > On 2017/01/06 04:18:49, haraken wrote:
> > > 
> > > I hit a link error if I put this in GCInfo. I couldn't work around it, so
> put
> > it
> > > outside the class declaration.
> > > 
> > 
> > Do you remember what the link error was?
> 
> See the error in PS3. It happens only on ChromeOS.

I don't see any link errors related to gcInfoMaxIndex there.

[haraken, 2017-01-08 13:45:38]

On 2017/01/08 13:31:17, sof wrote:
> On 2017/01/08 13:29:02, haraken wrote:
> > On 2017/01/08 07:10:47, sof wrote:
> > >
> >
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2619493003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/heap/GCInfo.h:165: static const size_t
> > > gcInfoMaxIndex = 1 << 14;
> > > On 2017/01/06 04:18:49, haraken wrote:
> > > > 
> > > > I hit a link error if I put this in GCInfo. I couldn't work around it,
so
> > put
> > > it
> > > > outside the class declaration.
> > > > 
> > > 
> > > Do you remember what the link error was?
> > 
> > See the error in PS3. It happens only on ChromeOS.
> 
> I don't see any link errors related to gcInfoMaxIndex there.

This one:

FAILED: libblink_platform.so libblink_platform.so.TOC 
python
"/usr/local/google/home/haraken/chromium/src/build/toolchain/gcc_solink_wrapper.py"
--readelf="readelf" --nm="nm"  --sofile="./libblink_platform.so"
--tocfile="./libblink_platform.so.TOC" --output="./libblink_platform.so"  --
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared
-Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro
-Wl,-z,defs -Wl,--no-as-needed -lpthread -Wl,--as-needed -fuse-ld=gold
-B../../third_party/binutils/Linux_x64/Release/bin -Wl,--threads
-Wl,--thread-count=4 -Wl,--icf=all -m64 -pthread -Werror -Wl,-O1
-Wl,--gc-sections --sysroot=../../build/linux/debian_wheezy_amd64-sysroot
-L/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/lib/x86_64-linux-gnu
-Wl,-rpath-link=/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/lib/x86_64-linux-gnu
-L/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu
-Wl,-rpath-link=/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu
-L/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6
-Wl,-rpath-link=/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6
-L/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib
-Wl,-rpath-link=/usr/local/google/home/haraken/chromium/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib
-Wl,--export-dynamic -o "./libblink_platform.so"
-Wl,-soname="libblink_platform.so" @"./libblink_platform.so.rsp"
../../base/logging.h:631: error: undefined reference to
'blink::GCInfoTable::maxIndex'
../../base/logging.h:631: error: undefined reference to
'blink::GCInfoTable::maxIndex'
../../base/logging.h:631: error: undefined reference to
'blink::GCInfoTable::maxIndex'
../../base/logging.h:631: error: undefined reference to
'blink::GCInfoTable::maxIndex'