Adding Danno because of my question about CompareNilICStub::Record.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc
File src/code-stubs.cc (right):
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode413
src/code-stubs.cc:413: if (nil_value_ == kNullValue) {
Use a ternary. More importantly: Why do we *set* types_ here and *Add*
below? It is unclear to me why we have to handle this differently. If it
is correct, please add a comment. (Yes, it has benn there before in a
different form, but nevertheless...)
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode443
src/code-stubs.cc:443: if (nil_value_ == kNullValue) {
Use a ternary ?: here...
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode448
src/code-stubs.cc:448: if (equality_kind_ == kStrictEquality) {
... and here, and merge the stream->Add() calls. More concise and easier
to read.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode462
src/code-stubs.cc:462: stream->Remove(1);
Although it looks like a cunning idea at first, removing things from a
stream is a bad idea conceptually. I want to refactor things to clearly
distinguish between the conversion of an object to a sequence of
characters and the character sink itself. This kind of hack would be
counter-productive. Just keep track of the fact if you need to emit a
separator by hand. C++ sucks:
http://hackage.haskell.org/packages/archive/base/latest/doc/html/Data-List.html#v:intersperse
:-/
"What happens in a stream, stays in a stream!". ;-)
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.cc#newcode582
src/code-stubs.cc:582: stream->Remove(1);
Same comment as in the other Print() function.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1063
src/code-stubs.h:1063: static const Types FullCompare() {
No need for "const" here, we have an implicit copy, anyway.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1071
src/code-stubs.h:1071: bool IsFullCompare() const {
No need for this function, we have FullCompare() and operator==().
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1075
src/code-stubs.h:1075: void SetTo(Type type);
We don't need this, pretending that Types is immutable and just
assigning is clearer. If you like, you could add a constructor/factory
method for a singleton to polish the call sites a bit, but that's up to
you.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1076
src/code-stubs.h:1076: byte ToByte() const { return ToIntegral(); }
Remove this, just use ToIntegral().
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1147
src/code-stubs.h:1147: const Types GetTypes() const { return types_; }
The first 'const' doesn't really make sense: It doesn't force the callee
to assign the result to e.g. a 'const Types foo'. Furthermore, we
implicitly copy types_, anyway.
https://codereview.chromium.org/14862009/diff/8001/src/code-stubs.h#newcode1844
src/code-stubs.h:1844: byte ToByte() const { return ToIntegral(); }
Just using ToIntegral() is easier here, too. (I know this has been there
before your change... :-)
https://codereview.chromium.org/14862009/
--
--
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.
