Hi Søren, Here is my first pass of comments. Overall, thanks for writing good code!
http://codereview.chromium.org/115024/diff/24/1005 File src/func-name-inferrer.cc (right): http://codereview.chromium.org/115024/diff/24/1005#newcode28 Line 28: #include "v8.h" Includes should really include a path. Is it possible for you to use "v8/src/v8.h"? http://codereview.chromium.org/115024/diff/24/1006 File src/func-name-inferrer.h (right): http://codereview.chromium.org/115024/diff/24/1006#newcode31 Line 31: namespace v8 { namespace internal { I have never seen anybody group namespace beginnings or endings like this. Looking through random Google code, it looks like normal practice is to have them on separate lines. http://codereview.chromium.org/115024/diff/24/1006#newcode137 Line 137: } } // namespace v8::internal I wouldn't put two spaces before this since you didn't above either. http://codereview.chromium.org/115024/diff/24/1007 File src/log.cc (right): http://codereview.chromium.org/115024/diff/24/1007#newcode345 Line 345: union Output { Can you explain this union, and when each member is valid. Personally, I'd probably not bother with the union since space isn't an issue (there's only one of them) and then you can initialize the unused one to NULL which will help catch errors. http://codereview.chromium.org/115024/diff/24/1008 File src/log.h (right): http://codereview.chromium.org/115024/diff/24/1008#newcode209 Line 209: static bool is_enabled(); Generally, naming like a variable should be used when calling the method is as efficient as accessing a variable. We don't usually consider non-inline functions to be like this, even if they're fast. I assume you did this to avoid getting the dependencies in the header, which is good. I'd just name this IsEnabled(). http://codereview.chromium.org/115024 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
