Hi Erik, I think I've addressed all your comments in patch set 2.
ry http://codereview.chromium.org/2092007/diff/1/2 File src/platform-solaris.cc (right): http://codereview.chromium.org/2092007/diff/1/2#newcode65 src/platform-solaris.cc:65: return x < 0; On 2010/05/15 20:00:02, Erik Corry wrote:
This will return 0 for a 'negative NaN'. That seems to be OK, since
everywhere
we use signbit we know it is not a NaN, but it at least deserves a
comment.
Alternatively i could be fixed. if (x < x) will tell you if you have
a NaN, I
think. signbit is part of C99 so presumably it is coming to Solaris
at some
point. Does the ifndef do anything?
I added some comments about why this is here and a note about -NaN. http://codereview.chromium.org/2092007/diff/1/2#newcode248 src/platform-solaris.cc:248: struct stack_walker { On 2010/05/15 20:00:02, Erik Corry wrote:
Unless this name is constrained by Solaris, which seems unlikely, it
should be
NamedLikeThis and not_like_this.
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode249 src/platform-solaris.cc:249: Vector<OS::StackFrame> &frames; On 2010/05/15 20:00:02, Erik Corry wrote:
Incorrect placement of & (see below).
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode254 src/platform-solaris.cc:254: static int StackWalkCallback(uintptr_t pc, int signo, void *data) { On 2010/05/15 20:00:02, Erik Corry wrote:
Incorrect placement of asterisk. The Google C++ style guide requires
us to be
consistent and AFAIR prefers asterisk next to the type rather than the
variable.
That is what we try to stick to in V8 (even though it is wrong :-).
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode255 src/platform-solaris.cc:255: struct stack_walker * walker = static_cast<struct stack_walker *>(data); On 2010/05/15 20:00:02, Erik Corry wrote:
No space before * in types (twice in this line).
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode260 src/platform-solaris.cc:260: walker->frames[i].address = (void*)pc; On 2010/05/15 20:00:02, Erik Corry wrote:
We don't allow C style casts. You have to use static_cast,
dynamic_cast or
const_cast.
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode268 src/platform-solaris.cc:268: if (dladdr((void*)pc, &info) == 0) { On 2010/05/15 20:00:02, Erik Corry wrote:
C style cast.
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode271 src/platform-solaris.cc:271: // we have containing symbol info On 2010/05/15 20:00:02, Erik Corry wrote:
Comments should start with a capital letter and end with a full stop
(period). Done. http://codereview.chromium.org/2092007/diff/1/2#newcode274 src/platform-solaris.cc:274: // no local symbol info On 2010/05/15 20:00:02, Erik Corry wrote:
Comment formatting.
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode278 src/platform-solaris.cc:278: (unsigned long)pc - (unsigned long)info.dli_fbase, On 2010/05/15 20:00:02, Erik Corry wrote:
C style casts and %p seems inappropriate for an unsigned long.
Done. http://codereview.chromium.org/2092007/diff/1/2#newcode292 src/platform-solaris.cc:292: if (!walkcontext(&ctx, StackWalkCallback, (void*)(&walker))) { On 2010/05/15 20:00:02, Erik Corry wrote:
C style cast.
Done. http://codereview.chromium.org/2092007/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
