https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc File src/profile-generator.cc (right):
https://chromiumcodereview.appspot.com/10353010/diff/1/src/profile-generator.cc#newcode1161 src/profile-generator.cc:1161: CHECK(root_entry_ == 0); On 2012/05/08 12:05:20, Mikhail Naganov (Chromium) wrote:
On 2012/05/07 11:04:35, alexeif wrote: > On 2012/05/03 15:44:31, Mikhail Naganov (Chromium) wrote: > > Are you using CHECKs here and below intentionally? They will
remain in
release > > mode. > I added them intentionally in places that are non performance
critical. Is it
> wrong? Is there a policy on ASSERT vs. CHECK usage?
My feeling is that as V8 is geared towards speed, in release mode we
don't do
any checks. Grepping for CHECK vs. ASSERT shows that CHECK is mostly
used in
tests and for some reason in ARM code generators. Also remember that
CHECK will
terminate execution. So unless you are thinking that there is some
potential
problem, and you would like to get crash reports from live users to
debug it, I
would stick to ASSERTs.
Done. https://chromiumcodereview.appspot.com/10353010/diff/7001/src/list.h File src/list.h (right): https://chromiumcodereview.appspot.com/10353010/diff/7001/src/list.h#newcode120 src/list.h:120: // Allocates space for 'length' elements. On 2012/05/08 12:05:20, Mikhail Naganov (Chromium) wrote:
nit: Please state that it also deletes current list contents.
Done. https://chromiumcodereview.appspot.com/10353010/diff/7001/src/profile-generator.h File src/profile-generator.h (right): https://chromiumcodereview.appspot.com/10353010/diff/7001/src/profile-generator.h#newcode489 src/profile-generator.h:489: // During entries population |to_| is used for storing the index, On 2012/05/08 12:05:20, Mikhail Naganov (Chromium) wrote:
nit: |to_| -> |to_index_|
Done. https://chromiumcodereview.appspot.com/10353010/diff/7001/src/profile-generator.h#newcode657 src/profile-generator.h:657: int root_entry_; On 2012/05/08 14:55:59, Yury Semikhatsky wrote:
I'd rename these fields from *_entry_ to *_index_
Done. https://chromiumcodereview.appspot.com/10353010/diff/7001/test/cctest/test-list.cc File test/cctest/test-list.cc (right): https://chromiumcodereview.appspot.com/10353010/diff/7001/test/cctest/test-list.cc#newcode138 test/cctest/test-list.cc:138: CHECK_EQ(100, list.length()); On 2012/05/08 12:05:20, Mikhail Naganov (Chromium) wrote:
Perhaps, also check that before allocate list[0] == 1, and afterwards
is not? The check for the first item being 1 after Add should be done in the test for Add operation. It doesn't make sense to do the check after Allocate because elements are not initialized and contain garbage. https://chromiumcodereview.appspot.com/10353010/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
