http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#newcode88
src/ia32/lithium-ia32.cc:88: for (UseIterator it = GetUseIterator();
it.HasNext(); ) {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
I don't think the GetUseIterator/GetTempIterator functions add much.
You could
just use constructor syntax here:
for (UseIterator it(this); it.HasNext(); ) { ... }
You might also consider separating the functions for getting the
current thing
and advancing to the next thing so you can write:
for (UseIterator it(this); it.HasNext(); it.Advance()) { ... }
so it reads better as a for loop (or keep HasNext's advancing
semantics and
write it as a while loop?
{ UseIterator it(this);
while (it.HasNext()) { ... }
}
).
Done.
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#newcode945
src/ia32/lithium-ia32.cc:945: chunk_->AddInstruction(instr,
current_block_);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
Since you now always ignore the return value, you can AddInstruction a
void
function (and simplify its implementation a bit).
Done.
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#newcode338
src/ia32/lithium-ia32.h:338: bool IsSaveDoubles() const { return
is_save_doubles_; }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
These two names are not entirely consistent: is_thing_ vs. is_action_;
MarkAsThing vs. MarkAsAction; and IsMarkedAsThing vs. IsAction.
I usually suggest is_thing_/IsThing() and
should_action_/ShouldAction(). And
IsMarkedAsThing() is just a longer way to say IsThing().
IsCall is already taken as the name of the type-tester - i can make both
IsMarkedAs...
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#newcode340
src/ia32/lithium-ia32.h:340: virtual bool HasResult() const = 0;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
const doesn't care if it has extra spaces around it, but I do!
Done.
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#newcode392
src/ia32/lithium-ia32.h:392: static T t = NULL;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
Do we really write NULL and not 0 to initialize a T?
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h
File src/lithium-allocator-inl.h (right):
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h#newcode52
src/lithium-allocator-inl.h:52: bool TempIterator::HasNext() { return
current_ < instr_->TempCount(); }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
I'm not sure it makes much difference, but I'd put the limit
(instr_->TempCount()) in a field of the iterator.
Good point. Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h#newcode92
src/lithium-allocator-inl.h:92: env_iterator_(instr->HasEnvironment() ?
instr->environment() : NULL) { }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
Using SetOncePointer for the environment is probably a mistake. From
SetOnecePointer's declaration: "A pointer that can only be set once
and doesn't
allow NULL values." This one effectively does allow NULL values.
LInstruction::HasEnvironment is the bad smell. We normally shouldn't
have to
ask a SetOncePointer if it's been set.
Done. Made it a normal pointer.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#newcode779
src/lithium-allocator.cc:779: for (TempIterator it =
first->GetTempIterator(); it.HasNext(); ) {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
for (TempIterator it(first); it.HasNext(); it.Advance()) { ... }
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#newcode780
src/lithium-allocator.cc:780: LUnallocated* temp =
LUnallocated::cast(it.Next());
On 2011/01/20 12:20:30, Kevin Millikin wrote:
There's an extra space after =.
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#newcode926
src/lithium-allocator.cc:926: LInstruction* instr =
chunk_->instructions()->at(index);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
It might be easier to read and write these sites if there was a simple
LChunk::InstructionAt(int) function.
Done. Introduces InstructionAt and GapAt helpers.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h
File src/lithium.h (right):
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode501
src/lithium.h:501: ASSERT(env_ != NULL);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
In normal use, should one ever call Next on an environment that is not
known to
HasNext()?
In other words, I would expect ASSERT(HasNext()) rather than checking
for it.
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode521
src/lithium.h:521: ShallowIterator shallow_iterator() { return
ShallowIterator(this); }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
This function (and deep_iterator) probably shouldn't have
hacker_style_naming.
It returns a fresh object on each call so it's not just an accessor,
and the
object has a non-trivial constructor.
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode529
src/lithium.h:529: if (current_env_ == NULL) return false;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
I think you can simplify this class's implementation a bit. The
current_env_ is
the environment where Next() will be found if HasNext() and NULL
otherwise. The
current_iterator_ is always a shallow iterator for the current
environment. The
shallow iterator for the NULL environment does not HasNext().
So:
inline bool HasNext() { return current_iterator_.HasNext(); }
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode536
src/lithium.h:536: inline LOperand* Next() {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
inline LOperand* Next() {
ASSERT(current_iterator_.HasNext());
LOperand* operand = current_iterator_.Next();
Advance();
return operand;
}
Done.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode544
src/lithium.h:544: inline void Advance() {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
inline void Advance() {
while (!current_iterator_.HasNext() && current_env_ != NULL) {
current_env_ = current_env_->outer();
current_iterator_ = ShallowIterator(current_env_);
}
}
Then add to the constructor a call to Advance();
Done.
http://codereview.chromium.org/6352006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
