Everything should have been addressed at this point...
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2592
src/hydrogen.cc:2592: if (check->index()->IsAdd()) {
On 2012/04/13 11:38:06, Sven Panne wrote:
If we extract a function like 'MatchOffsetExpresssion(HValue**
index_base,
HConstant** constant)' and massage CoverCheck a tiny bit, it should be
straightforward to handle the HMinus case, too. I leave it up to you
to do this
now or in a separate CL.
I'll do it in a separate CL.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2637
src/hydrogen.cc:2637: // class BoundsCheckBbData {
On 2012/04/13 11:38:06, Sven Panne wrote:
Remove that line.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2639
src/hydrogen.cc:2639: BoundsCheckKey* Key() {return key_;}
On 2012/04/13 12:38:00, fschneider wrote:
style-nit: add spaces after/before {}.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2646
src/hydrogen.cc:2646: static void RemoveCheck(HBoundsCheck* check) {
On 2012/04/13 11:38:06, Sven Panne wrote:
I think this method should be generalized and moved to
HTemplateInstruction
(under a better name). In general, static methods are a code smell,
with very
few exceptions like factory methods.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2659
src/hydrogen.cc:2659: if (check_simulate_ != NULL) {
On 2012/04/13 11:38:06, Sven Panne wrote:
Use a guard clause instead, see
http://martinfowler.com/refactoring/catalog/replaceNestedConditionalWithGuardClauses.html
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2671
src/hydrogen.cc:2671: added_index_->InsertBefore(added_simulate);
I ended up asserting that the index representation is int32 because it
must be true and it's enough for what we need.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2682
src/hydrogen.cc:2682: BoundsCheckBbData(BoundsCheckKey* key, int32_t
offset, HBasicBlock* bb,
On 2012/04/13 12:38:00, fschneider wrote:
style-nit: One parameter pre line in definitions and declarations.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2689
src/hydrogen.cc:2689: if (check->next()->IsSimulate()) {
On 2012/04/13 11:38:06, Sven Panne wrote:
Using a ternary operator here and using initializer syntax instead is
more
"C++"-like.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2720
src/hydrogen.cc:2720: intptr_t hash = key->Hash();
On 2012/04/13 11:38:06, Sven Panne wrote:
Wrong type, but this is nicer when inlined, anyway.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2726
src/hydrogen.cc:2726: intptr_t hash = key->Hash();
On 2012/04/13 11:38:06, Sven Panne wrote:
Inline again.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2727
src/hydrogen.cc:2727: ZoneHashMap::Entry* entry = Lookup(key, hash,
true);
On 2012/04/13 11:38:06, Sven Panne wrote:
No need to name this explicitly.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2749
src/hydrogen.cc:2749: if (i->IsBoundsCheck()) {
On 2012/04/13 11:38:06, Sven Panne wrote:
'if (!i->IsBoundsCheck()) continue;' reduces nesting a bit and is
clearer IMHO.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2769
src/hydrogen.cc:2769: BoundsCheckBbData::RemoveCheck(check);
On 2012/04/13 11:38:06, Sven Panne wrote:
This will probably be something like 'check->Remove()' or whatever the
function
will be called.
Done.
https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#newcode2773
src/hydrogen.cc:2773:
isolate()->counters()->array_bounds_checks_removed()->Increment();
On 2012/04/13 11:38:06, Sven Panne wrote:
I don't think that the increment here is correct, because the check is
removed
conditionally.
Done.
https://chromiumcodereview.appspot.com/10032029/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
