Thanks, Brett! Code is getting better and better. I understand that I'll need to upstream these changes. Hope that Git will help me. I just need to say that prior to change namespaces declaration across the whole project I will need to get an approval from the team. But as this is a mechanical change and it will make sources more style-compliant, I think this will not be a problem.
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); On 2009/05/19 20:22:41, brettw wrote: > 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. Fixed---made private. http://codereview.chromium.org/115024/diff/1021/1026#newcode349 Line 349: union Output { On 2009/05/19 20:22:41, brettw wrote: > 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. You have convinced me. Actually, I already had a bug when I called GetLogLines on a Log opened with OpenFile. Replaced union with two fields and made according changes to member functions. 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; On 2009/05/19 20:22:41, brettw wrote: > 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. I've got rid of this namespace alias, but want to mention that this idiom isn't uncommon in V8: grep for "namespace = i" and you'll find a couple of entries: src/d8.h:38:namespace i = v8::internal; src/mksnapshot.cc:42:namespace i = v8::internal; src/v8.h:95:namespace i = v8::internal; test/cctest/test-api.cc:59:namespace i = ::v8::internal; Also namespace aliases are allowed by the style guide too: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces (see the very end of the section). http://codereview.chromium.org/115024 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
