Hi Brett, I've made fixed according to your suggestions.
(BTW, you accidentally confused me with Søren who was an initial reviewer). 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" On 2009/05/18 20:12:45, brettw wrote: > Includes should really include a path. Is it possible for you to use > "v8/src/v8.h"? As I see in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_and_Order_of_Includes#Names_and_Order_of_Includes "All of a project's header files should be listed as descentants of the project's source directory... For example, google-awesome-project/src/base/logging.h should be included as #include "base/logging.h"". If we replace "google-awesome-project" with "v8" (and "base/" with ""), it will result in "#include "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 { On 2009/05/18 20:12:45, brettw wrote: > 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. I agree with you that having each namespace on its own line is more readable. I've made changes in this CL, but in order to propagate through the whole V8's codebase, I need to ask the team first. http://codereview.chromium.org/115024/diff/24/1006#newcode137 Line 137: } } // namespace v8::internal On 2009/05/18 20:12:45, brettw wrote: > I wouldn't put two spaces before this since you didn't above either. This is about vertical space, right? Fixed. 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 { On 2009/05/18 20:12:45, brettw wrote: > 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. Added more comments. I prefer using union here because cases of using file or buffer for logging are mutually exclusive, and usage of union emphasizes this. Also, as all pointers have the same size, it is safe to work with any member of this union, to be sure that other member is also touched. Thus, instead of writing: ASSERT(output_.handle == NULL); ASSERT(output_.buffer == NULL); for checking that all members are NULL, or output_.handle = NULL; output_.buffer = NULL; to reset all members, as it would be needed with a struct, I can use only one statement and be sure that other member is also checked or reset. 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(); On 2009/05/18 20:12:45, brettw wrote: > 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(). Good catch! Fixed. http://codereview.chromium.org/115024 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
