Changelist for a readability review. Not for direct commit.

Changelist for a readability review. Not for direct commit.

I've combined main parts of issues 62146 and 108011 in order to obtain a C++ readability.

I've merged readability changes:
http://codereview.chromium.org/113763
and another one with nested namespaces formatting:
http://codereview.chromium.org/115756

[Mikhail Naganov, 2009-05-06 13:02:17]

Hi Søren,

Can you please take another look at this code (from already submitted
changelists) before I apply for C++ readability.

[Søren Thygesen Gjesse, 2009-05-06 16:10:35]

A few comments with reference to the public Google C++ Style Guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml).

When submitting this for readability review remember to note that this is an
open source project where all review comments will become public. Also provide
some background on things like BASE_EMBEDDED and the TEST macro.

http://codereview.chromium.org/115024/diff/1001/16
File src/func-name-inferrer.h (right):

http://codereview.chromium.org/115024/diff/1001/16#newcode45
Line 45: FuncNameInferrer() :
: on the next line
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...)

http://codereview.chromium.org/115024/diff/1001/16#newcode52
Line 52: bool IsOpen() const { return !entries_stack_.is_empty(); }
All function declarations should have a comment
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

http://codereview.chromium.org/115024/diff/1001/16#newcode102
Line 102: explicit ScopedFuncNameInferrer(FuncNameInferrer* inferrer) :
: on the next line
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...)

http://codereview.chromium.org/115024/diff/1001/17
File src/log.cc (right):

http://codereview.chromium.org/115024/diff/1001/17#newcode304
Line 304: // See description in v8.h.
v8.h -> include/v8.h as v8.h is ambiguous.

http://codereview.chromium.org/115024/diff/1001/17#newcode310
Line 310: private:
Blank line before private:
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...)

[Mikhail Naganov, 2009-05-07 08:11:11]

Thank you very much!

http://codereview.chromium.org/115024/diff/1001/16
File src/func-name-inferrer.h (right):

http://codereview.chromium.org/115024/diff/1001/16#newcode45
Line 45: FuncNameInferrer() :
On 2009/05/06 16:10:35, Søren Gjesse wrote:
> : on the next line
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...)

Done.

http://codereview.chromium.org/115024/diff/1001/16#newcode52
Line 52: bool IsOpen() const { return !entries_stack_.is_empty(); }
On 2009/05/06 16:10:35, Søren Gjesse wrote:
> All function declarations should have a comment
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

http://codereview.chromium.org/115024/diff/1001/16#newcode102
Line 102: explicit ScopedFuncNameInferrer(FuncNameInferrer* inferrer) :
On 2009/05/06 16:10:35, Søren Gjesse wrote:
> : on the next line
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...)

Done.

http://codereview.chromium.org/115024/diff/1001/17
File src/log.cc (right):

http://codereview.chromium.org/115024/diff/1001/17#newcode304
Line 304: // See description in v8.h.
On 2009/05/06 16:10:35, Søren Gjesse wrote:
> v8.h -> include/v8.h as v8.h is ambiguous.
> 

Done.

http://codereview.chromium.org/115024/diff/1001/17#newcode310
Line 310: private:
On 2009/05/06 16:10:35, Søren Gjesse wrote:
> Blank line before private:
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...)

Done.

[brettw, 2009-05-18 20:12:45]

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().

[Mikhail Naganov, 2009-05-19 14:10:35]

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_...

"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.

[brettw, 2009-05-19 19:38:18]

I'm starting another pass on this. Note that you'll need to check in the changes
you made in this review to get readability. I'm not sure what the best way to do
that is from this code review (you could even apply the delta in a new CL).

[brettw, 2009-05-19 20:22:41]

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.

[Mikhail Naganov, 2009-05-20 13:29:29]

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=Namesp...
(see the very end of the section).

[brettw, 2009-05-20 16:56:16]

LGTM, thanks!

When you check this in, please ping me and I'll mark the bug fixed.

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/20 13:29:30, Mikhail Naganov wrote:
> 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=Namesp...
> (see the very end of the section).

I learn something every time! If you do this, I think "i" is a bad namespace
alias, though, given the common use of i for other things.

[iposva, 2009-05-20 17:41:25]

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/20 16:56:16, brettw wrote:
> On 2009/05/20 13:29:30, Mikhail Naganov wrote:
> > 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=Namesp...
> > (see the very end of the section).
> 
> I learn something every time! If you do this, I think "i" is a bad namespace
> alias, though, given the common use of i for other things.

Coming to Mikhail's defense here. One of the style guide's principles is to stay
consistent with existing code. The namespace i:: use is rather common in the
rest of the V8 code where we need to refer to the v8::internal namespace,
although I like Mikhail's current solution better because it specifically names
the internal classes his test code is using.

-Ivan