https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2309
src/hydrogen.h:2309: class HUnique V8_FINAL {
A separate file would be nice.

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2316
src/hydrogen.h:2316: raw_address_ = reinterpret_cast<Address>(*handle);
Can we ensure that this is only called in a DisallowHeapAllocation? This
is pretty scary stuff.

The UniqueValueIds are in exactly such a scope, so they are already
protected. If you cannot do this because you plan on using this in
parallel compilation; could we just use the unique value ids that are
stored up front rather than the handles?

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2383
src/hydrogen.h:2383: Grow(size_ + 1, zone);
You can merge this code with the code after the for-loop by declaring
"int i" outside of the for-loop; then always growing by 1, moving
elements past i to the top (won't be any if i == size), and assigning
array_[i] = uniq;

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2419
src/hydrogen.h:2419: if (!found) return false;
For minimal checking

if (that->size_ < this->size_) return false;
int j = 0;
for (int i = 0; i < this->size_; i++) {
  HUnique<T> sought = this->array_[i];
  do {
    if (sought == that->array[j++]) break;
    // Fail whenever we have less elements left in [that] than
    // we still need to find from [this].
    if (that->size_ - j < this->size_ - i) return false;
  } while (true);
}
return true;

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2429
src/hydrogen.h:2429: out->Grow(this->size_ > that->size_ ? this->size_ :
that->size_, zone);
I guess you meant to flip the sizes?
What about just using out->Grow(Min(this->size_, that->size_), zone) to
avoid problems :)

https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2518
src/hydrogen.h:2518: int new_capacity = capacity_ + capacity_ + size;
// 2*current + needed
I guess this is 0, 1 or 4? Otherwise it seems like a lot of extra
space...

https://codereview.chromium.org/23609020/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Reply via email to