I've got lots of comments about the iterator. The biggest one is I usually
like
to expose Advance() and not have it rolled into Next().
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(); ) {
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()) { ... }
}
).
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#newcode945
src/ia32/lithium-ia32.cc:945: chunk_->AddInstruction(instr,
current_block_);
Since you now always ignore the return value, you can AddInstruction a
void function (and simplify its implementation a bit).
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_; }
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().
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#newcode340
src/ia32/lithium-ia32.h:340: virtual bool HasResult() const = 0;
const doesn't care if it has extra spaces around it, but I do!
http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#newcode392
src/ia32/lithium-ia32.h:392: static T t = NULL;
Do we really write NULL and not 0 to initialize a T?
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#newcode33
src/lithium-allocator-inl.h:33: #if V8_TARGET_ARCH_IA32
Ultimately (not as part of this change), I'd like to have lithium.h
include the appropriate lithium-ia32.h. There should be something one
merely includes to get the correct thing, without having to test at each
point it wants to be included.
Here I guess you actually want the lithium instructions, so fixing this
site is blocked by renaming ia32/lithium-ia32.h to
ia32/lithium-instructions.h. Then you can have a platform independent
"lithium-instructions.h" that just includes the appropriate
platform-specific one and here just write:
#include "lithium-instructions.h"
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(); }
I'm not sure it makes much difference, but I'd put the limit
(instr_->TempCount()) in a field of the iterator.
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) { }
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.
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(); ) {
for (TempIterator it(first); it.HasNext(); it.Advance()) { ... }
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#newcode780
src/lithium-allocator.cc:780: LUnallocated* temp =
LUnallocated::cast(it.Next());
There's an extra space after =.
http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#newcode926
src/lithium-allocator.cc:926: LInstruction* instr =
chunk_->instructions()->at(index);
It might be easier to read and write these sites if there was a simple
LChunk::InstructionAt(int) function.
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);
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.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode521
src/lithium.h:521: ShallowIterator shallow_iterator() { return
ShallowIterator(this); }
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.
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode529
src/lithium.h:529: if (current_env_ == NULL) return false;
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(); }
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode536
src/lithium.h:536: inline LOperand* Next() {
inline LOperand* Next() {
ASSERT(current_iterator_.HasNext());
LOperand* operand = current_iterator_.Next();
Advance();
return operand;
}
http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode544
src/lithium.h:544: inline void Advance() {
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();
http://codereview.chromium.org/6352006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
