First round of comments, already looking awesome.
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc
File src/allocation-site-scopes.cc (right):
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode28
src/allocation-site-scopes.cc:28: #include "allocation-site-scopes.h"
newline
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode29
src/allocation-site-scopes.cc:29: #define PTR(x)
reinterpret_cast<void*>((x))
I am not a fan of macros, can we just use the cast below?
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode40
src/allocation-site-scopes.cc:40: // top level
Make a proper comment out of this comment or remove it.
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode99
src/allocation-site-scopes.cc:99: // Advance current if activated
finish with "."
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode102
src/allocation-site-scopes.cc:102: // Something is wrong if we advance
to the end of the list here
finish with "."
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode128
src/allocation-site-scopes.cc:128: // in the allocation site
finish with "."
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.h
File src/allocation-site-scopes.h (right):
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.h#newcode33
src/allocation-site-scopes.h:33: #include "objects.h"
alphabetic order
https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc#newcode8562
src/hydrogen.cc:8562: // site_context->top()->GetPretenureMode().
Cool!
https://codereview.chromium.org/24250005/diff/27001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/24250005/diff/27001/src/objects.cc#newcode5634
src/objects.cc:5634: // 1) we have a JSArray, and
What about JSObject?
https://codereview.chromium.org/24250005/diff/27001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode2029
src/objects.h:2029: class AllocationSiteContext;
Move this one up to the other forward class declarations.
https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode7910
src/objects.h:7910: return transition_info()->IsJSArray() ||
transition_info()->IsJSObject();
The method name and the description is outdated.
https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc#newcode516
src/runtime.cc:516: AllocationSiteUsageScope site_scope(&site_context,
site, boilerplate);
As discussed offline, right now we would emit mementos also in optimized
code. We should turn that off (in a different cl if it does not tank
performance).
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc
File src/allocation-site-scopes.cc (right):
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode29
src/allocation-site-scopes.cc:29: #define PTR(x)
reinterpret_cast<void*>((x))
Again this macro, I would prefer using the cast. Or if you really like
the macro, add it to a global include file.
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode40
src/allocation-site-scopes.cc:40: // top level
Make a proper comment out of it.
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode62
src/allocation-site-scopes.cc:62: *context->current_.location() =
*new_site;
Can we use a setter for that?
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode99
src/allocation-site-scopes.cc:99: // Advance current if activated
finish comment with "."
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode102
src/allocation-site-scopes.cc:102: // Something is wrong if we advance
to the end of the list here
finish comment with "."
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode105
src/allocation-site-scopes.cc:105: *context->current_.location() =
casted_site;
Can we use a setter for that?
https://codereview.chromium.org/24250005/
--
--
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/groups/opt_out.
