https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc
File src/compiler/coalesced-live-ranges.cc (right):
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode66
src/compiler/coalesced-live-ranges.cc:66: break;
On 2015/07/21 08:51:18, jarin wrote:
This continue-break gymnastics looks awkward.
How about
if (pos_ != end) {
DCHECK(QueryIntersectsAllocatedInterval());
break;
}
Or possibly
if (pos_ != end) {
DCHECK(QueryIntersectsAllocatedInterval());
return;
}
(Then you can unconditionally Invalidate after the loop.)
Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode96
src/compiler/coalesced-live-ranges.cc:96:
storage_->erase({interval->start(), interval->end(), nullptr});
On 2015/07/21 08:51:18, jarin wrote:
Uniform initialization is banned in Chrome, apparently. Please equip
AllocationInterval with a constructor.
(See https://chromium-cpp.appspot.com. The reason for banning is
incorrect
support in some versions of Visual Studio.)
Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode119
src/compiler/coalesced-live-ranges.cc:119:
storage().insert({interval->start(), interval->end(), range});
On 2015/07/21 08:51:18, jarin wrote:
Same here, no uniform initialization.
Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h
File src/compiler/coalesced-live-ranges.h (right):
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode48
src/compiler/coalesced-live-ranges.h:48: LiveRange*
ClearCurrentAndGetNext() { return InternalGetNext(true); }
On 2015/07/21 08:51:18, jarin wrote:
ClearCurrent seems ambiguous to me, it sounds like it is somehow
invalidating
the current range.
How about EvictCurrentAndGetNext, then? (Or RemoveCurrentAndGetNext.)
Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode100
src/compiler/coalesced-live-ranges.h:100: IntervalStore* storage_;
On 2015/07/21 08:51:18, jarin wrote:
Could we please move away from the 'storage_' name? It does not say at
all what
the field is (every field is in essence storage). How about
intervals_?
Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode131
src/compiler/coalesced-live-ranges.h:131: bool
TestOnlyVerifyAllocationsAreValid() const;
On 2015/07/21 08:51:18, jarin wrote:
TestOnlyVerifyAllocationsAreValid ->
VerifyAllocationsAreValidForTesting
The Chrome convention is to add the ForTesting suffix (and there are
some rough
checks that aim to detect calls from production code to testing code
(i.e.,
methods without the ForTesting suffix calling methods with the
ForTesting
suffix).
Ah, it was a suffix. Done.
https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode136
src/compiler/coalesced-live-ranges.h:136: IntervalStore& storage() {
return storage_; }
On 2015/07/21 08:51:18, jarin wrote:
Again, could we rename storage -> intervals.
Done.
https://codereview.chromium.org/1219063017/diff/180001/test/unittests/compiler/coalesced-live-ranges-unittest.cc
File test/unittests/compiler/coalesced-live-ranges-unittest.cc (right):
https://codereview.chromium.org/1219063017/diff/180001/test/unittests/compiler/coalesced-live-ranges-unittest.cc#newcode36
test/unittests/compiler/coalesced-live-ranges-unittest.cc:36: auto pair
= pairs_[i];
On 2015/07/21 08:51:18, jarin wrote:
auto -> Interval
Done.
https://codereview.chromium.org/1219063017/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
