Provide constant values for field and extension numbers, with unit tests.

[kenton, 2009-04-16 23:08:00]

Mostly looks good.  Can you address the comments below and upload a new patch?

Thanks for doing this.

http://codereview.appspot.com/40081/diff/1/2
File java/src/test/java/com/google/protobuf/GeneratedMessageTest.java (right):

http://codereview.appspot.com/40081/diff/1/2#newcode509
Line 509:
assertEquals(UnittestProto.OPTIONAL_NESTED_MESSAGE_EXTENSION_EXTENSION_NUMBER,
18);
Please make sure your code fits in 80 columns, so that people don't have to read
wrapped lines or scroll horizontally.  (This comment applies to all the files in
this patch.)

http://codereview.appspot.com/40081/diff/1/3
File python/google/protobuf/reflection.py (right):

http://codereview.appspot.com/40081/diff/1/3#newcode334
Line 334: constant_name = field.name.upper() + "_FIELD_NUMBER"
Hmm, should we do extensions too just for consistency with other languages?

http://codereview.appspot.com/40081/diff/1/15
File src/google/protobuf/compiler/cpp/cpp_enum_field.cc (right):

http://codereview.appspot.com/40081/diff/1/15#newcode108
Line 108: "const int $classname$::$constant_name$;\n");
Since the exact same constant is defined for every field type, I think you
should just generate these inside message.cc.  That way you don't need to make
the same change to every implementation of FieldGenerator.

Same comment applies to Java.

http://codereview.appspot.com/40081/diff/1/20
File src/google/protobuf/compiler/cpp/cpp_extension.cc (right):

http://codereview.appspot.com/40081/diff/1/20#newcode112
Line 112: // Likewise, class members need to declare the field constant
variable.
MSVC doesn't follow the standard here.  It actually won't work if you try to
define the member in the .cc file when its value is assigned in the .h file. 
So, you need to protect this with #ifndef _MSC_VER.  You should actually print
the #ifndef as part of the output, so that protoc itself generates the exact
same output regardless of the host system.

Same comment applies to non-extension fields.

http://codereview.appspot.com/40081/diff/1/14
File src/google/protobuf/compiler/cpp/cpp_helpers.h (right):

http://codereview.appspot.com/40081/diff/1/14#newcode70
Line 70: string ExtensionConstantName(const FieldDescriptor *extension);
IMO we might as well use the same constant naming convention for extensions and
fields.  Really, both are fields, but extensions are just a special kind of
field.  So they can both use "kFooFieldNumber".  (Same comment for Java.)

http://codereview.appspot.com/40081/diff/1/10
File src/google/protobuf/compiler/java/java_extension.cc (right):

http://codereview.appspot.com/40081/diff/1/10#newcode78
Line 78: "public static final int $constant_name$ = $number$;\n");
pedantic:  This should only be indented two extra spaces compared to the
previous line.

[mdpoole, 2009-04-17 01:03:45]

Five out of six done.  The last involves questions on my part about patch
management and design philosophy (none of the other classes seem to use
"protected").

http://codereview.appspot.com/40081/diff/1/2
File java/src/test/java/com/google/protobuf/GeneratedMessageTest.java (right):

http://codereview.appspot.com/40081/diff/1/2#newcode509
Line 509:
assertEquals(UnittestProto.OPTIONAL_NESTED_MESSAGE_EXTENSION_EXTENSION_NUMBER,
18);
On 2009/04/16 23:08:00, kenton wrote:
> Please make sure your code fits in 80 columns, so that people don't have to
read
> wrapped lines or scroll horizontally.  (This comment applies to all the files
in
> this patch.)

Done.

http://codereview.appspot.com/40081/diff/1/3
File python/google/protobuf/reflection.py (right):

http://codereview.appspot.com/40081/diff/1/3#newcode334
Line 334: constant_name = field.name.upper() + "_FIELD_NUMBER"
On 2009/04/16 23:08:00, kenton wrote:
> Hmm, should we do extensions too just for consistency with other languages?

Done.  This ended up also touching python_generator.cc to handle top-level
(non-nested) extensions.

http://codereview.appspot.com/40081/diff/1/15
File src/google/protobuf/compiler/cpp/cpp_enum_field.cc (right):

http://codereview.appspot.com/40081/diff/1/15#newcode108
Line 108: "const int $classname$::$constant_name$;\n");
On 2009/04/16 23:08:00, kenton wrote:
> Since the exact same constant is defined for every field type, I think you
> should just generate these inside message.cc.  That way you don't need to make
> the same change to every implementation of FieldGenerator.
> 
> Same comment applies to Java.

I assume this change should address the TODO comments in various *_field.cc
files about factoring out a "SetCommonFieldVariables()" method.  Should I submit
that as a separate code review and rebase this patch to apply on top of that, or
just add separate patches to this review for SetCommonFieldVariables() and this
set?  That would imply that variables_ would need to move up to the
FieldGenerator class -- presumably as a protected member?

http://codereview.appspot.com/40081/diff/1/20
File src/google/protobuf/compiler/cpp/cpp_extension.cc (right):

http://codereview.appspot.com/40081/diff/1/20#newcode112
Line 112: // Likewise, class members need to declare the field constant
variable.
On 2009/04/16 23:08:00, kenton wrote:
> MSVC doesn't follow the standard here.  It actually won't work if you try to
> define the member in the .cc file when its value is assigned in the .h file. 
> So, you need to protect this with #ifndef _MSC_VER.  You should actually print
> the #ifndef as part of the output, so that protoc itself generates the exact
> same output regardless of the host system.
> 
> Same comment applies to non-extension fields.

Done.  I'll make sure to test under MSVC in the future.

http://codereview.appspot.com/40081/diff/1/14
File src/google/protobuf/compiler/cpp/cpp_helpers.h (right):

http://codereview.appspot.com/40081/diff/1/14#newcode70
Line 70: string ExtensionConstantName(const FieldDescriptor *extension);
On 2009/04/16 23:08:00, kenton wrote:
> IMO we might as well use the same constant naming convention for extensions
and
> fields.  Really, both are fields, but extensions are just a special kind of
> field.  So they can both use "kFooFieldNumber".  (Same comment for Java.)

Done.

http://codereview.appspot.com/40081/diff/1/10
File src/google/protobuf/compiler/java/java_extension.cc (right):

http://codereview.appspot.com/40081/diff/1/10#newcode78
Line 78: "public static final int $constant_name$ = $number$;\n");
On 2009/04/16 23:08:00, kenton wrote:
> pedantic:  This should only be indented two extra spaces compared to the
> previous line.

Done.

[mdpoole, 2009-04-17 02:53:38]

http://codereview.appspot.com/40081/diff/1/15
File src/google/protobuf/compiler/cpp/cpp_enum_field.cc (right):

http://codereview.appspot.com/40081/diff/1/15#newcode108
Line 108: "const int $classname$::$constant_name$;\n");
On 2009/04/17 01:03:45, mdpoole wrote:
> On 2009/04/16 23:08:00, kenton wrote:
> > Since the exact same constant is defined for every field type, I think you
> > should just generate these inside message.cc.  That way you don't need to
make
> > the same change to every implementation of FieldGenerator.
> > 
> > Same comment applies to Java.
> 
> I assume this change should address the TODO comments in various *_field.cc
> files about factoring out a "SetCommonFieldVariables()" method.  Should I
submit
> that as a separate code review and rebase this patch to apply on top of that,
or
> just add separate patches to this review for SetCommonFieldVariables() and
this
> set?  That would imply that variables_ would need to move up to the
> FieldGenerator class -- presumably as a protected member?

Please ignore my previous comment here - after further time to stew, I see what
you mean.  New patch coming up.

[mdpoole, 2009-04-17 04:16:55]

I'll have to remember to explicitly specify a message file with upload.py.  It
didn't prompt me to provide one.  Apologies for the spam.

Patch set 2 should implement all of your requests, except that extension
constants are still emitted by (lang)_extension.cc rather than separately by
(lang)_message.cc and (lang)_file.cc (for nested and top-level extensions
respectively).

[kenton, 2009-04-18 00:02:24]

I made one small change before submitting:  I grouped all of the field number
declarations for each message together in the .pb.cc file so that they could
have a single #ifdef _MSC_VER around them.

Thanks again for doing this.

Committed as rev 109.