[ Sorry for the delay, this CL slipped between the cracks somehow... ]
First round of comments. Furthermore, you need to rebase, the CL doesn't
merge
cleanly.
https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi
File build/standalone.gypi (right):
https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi#newcode154
build/standalone.gypi:154: 'visibility%': '',
What's the reason for this special case?
https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
File src/assembler.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
src/assembler.cc:1461: #elif V8_OS_AIX
Can we merge this with the case above? If I see this correctly, only the
copysign is new, and this should be OK above, too (I hope :-).
https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h
File src/base/atomicops.h (right):
https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h#newcode60
src/base/atomicops.h:60: typedef Atomic32 AtomicWord;
Hmmm, why do we need this?
https://codereview.chromium.org/866843003/diff/1/src/base/logging.h
File src/base/logging.h (right):
https://codereview.chromium.org/866843003/diff/1/src/base/logging.h#newcode80
src/base/logging.h:80: #if V8_OS_AIX && V8_HOST_ARCH_32_BIT
Can we turn this into something less fragile (e.g. compile-time tests
regarding int sizes)? Why do we need this at all? Is intptr_t different
from int *and* int64_t? If yes, what is it?
As a side note, we should really get rid of this CheckEqualsHelper
nonsense, we tripped over this several times already.
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-aix.cc
File src/base/platform/platform-aix.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-aix.cc#newcode46
src/base/platform/platform-aix.cc:46: // converted to double precision
numbers.
Huh? Where do we convert addresses to doubles?
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-aix.cc#newcode164
src/base/platform/platform-aix.cc:164: int rc = read(fd, addr_buffer +
2, 8);
Does "read" really return an int on AIX? I can't find any online manual
pages for the AIX system calls (or the POSIX emulation).
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-posix.cc
File src/base/platform/platform-posix.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-posix.cc#newcode178
src/base/platform/platform-posix.cc:178: // b) avoid losing precision
if address is stored as a double.
Same question again: Where do we store addresses as doubles?
https://codereview.chromium.org/866843003/diff/1/src/codegen.cc
File src/codegen.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/codegen.cc#newcode57
src/codegen.cc:57: double result;
Don't use a declaration without initialization here and below, we're in
C++ land.
https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.cc
File src/compiler/graph-builder.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.cc#newcode5
src/compiler/graph-builder.cc:5: #if defined(_AIX)
Use V8_OS_AIX
https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.cc#newcode6
src/compiler/graph-builder.cc:6: #include <alloca.h>
Why do we need this? I don't see any alloca call here.
https://codereview.chromium.org/866843003/diff/1/src/d8.cc
File src/d8.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
src/d8.cc:66: #define malloc __linux_malloc
Why do we need this? A comment would be nice.
https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc
File src/disassembler.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc#newcode132
src/disassembler.cc:132: #else
Could you restructure these #if/#elif/#else/#endifs a bit, even if a few
lines might be duplicated? It's not readable as it is, having unbalanced
things in preprocessor directives is in general not a good idea.
https://codereview.chromium.org/866843003/diff/1/src/globals.h
File src/globals.h (right):
https://codereview.chromium.org/866843003/diff/1/src/globals.h#newcode42
src/globals.h:42: #undef V8_INFINITY
No #undef, please, move this up to the place where V8_INFINITY is
#defined.
https://codereview.chromium.org/866843003/diff/1/src/sampler.cc
File src/sampler.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode16
src/sampler.cc:16: #if !(V8_OS_QNX || V8_OS_AIX) && !V8_OS_NACL
De Morgan's law, please. :-)
https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode315
src/sampler.cc:315: // Ensure delivery of any pending SIGPROF before
removing the handler
Hmmm, why is this AIX-specific? Why can't there be yet another SIGPROF
after the handler and coming back to the yield?
In a nutshell: This looks wrong, like 99.9% of all yield uses I've seen
so far. :-) If it's really OK, we need an extensive comment here.
https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):
https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status#newcode251
test/mjsunit/mjsunit.status:251: 'asm/embenchen/*': [SKIP],
Do really all 9 embenchen tests fail on big-endian?
https://codereview.chromium.org/866843003/
--
--
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/d/optout.
