Thanks, here is my next round of comments. I think the include paths are wrong, and you should at least say "src/foo.h" since the root of your repo is v8. But there have been many arguments about this and I guess we're not going to fix it in this CL.
http://codereview.chromium.org/115024/diff/1021/1026 File src/log.cc (right): http://codereview.chromium.org/115024/diff/1021/1026#newcode311 Line 311: typedef int (*WritePtr)(const char* msg, int length); Are you sure this has to be public? If it is, it should be at the top of the public section since it's a type. Otherwise, it should be moved to the top of the private section. http://codereview.chromium.org/115024/diff/1021/1026#newcode349 Line 349: union Output { I think the union is unnecessary and potentially dangerous. I think they're also somewhat hard to understand conceptually, so shouldn't be used to show that two things happen to be mutually exclusive. Unions simply aren't used very often at Google, and should only be used when necessary. If you have a bug in our code where you're incorrectly accessing the wrong type, you'll either corrupt the file structure in hard-to-track-down ways by writing a string into it, or you'll crash mysteriously in the stdio code when it's trying to interpret your string as a FILE structure. With separate members, you will crash with a NULL pointer. http://codereview.chromium.org/115024/diff/1021/1028 File test/cctest/test-func-name-inference.cc (right): http://codereview.chromium.org/115024/diff/1021/1028#newcode42 Line 42: namespace i = ::v8::internal; I didn't know you could even do this, and I looked at a couple other test files and they just say "using". I think i is particularly confusing since it's often used as a loop index (I realize they won't collide). I realize you're trying to be nicer not bringing all of v8 into your scope, but I think using or saying v8::internal everywhere are more clear than this uncommon syntax. http://codereview.chromium.org/115024 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
