Left some comments. I really like this patch. Tested it with node.js
master
and it works great (as long as V8 can write to /tmp :-))
https://codereview.chromium.org/70013002/diff/40001/src/log.cc
File src/log.cc (right):
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode289
src/log.cc:289: perf_output_handle_ = OS::FOpen(perf_dump_name.start(),
OS::LogFileOpenMode);
This should check for NULL, shouldn't it? Calling setvbuf() or fclose()
on a NULL stream will segfault.
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode307
src/log.cc:307: (uint64_t)code->instruction_start(),
static_cast? (Here and in a few other places.)
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode452
src/log.cc:452: ASSERT(static_cast<size_t>(size) == rv);
I don't think that's a safe assumption to make. fwrite() can (and will)
sometimes write fewer bytes than requested, e.g. when the write() system
call got interrupted by a signal.
Minor nit: you probably meant `ssize_t` one line up.
https://codereview.chromium.org/70013002/
--
--
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.
