Re: [protobuf] Re: 2.3.0 released

2010-01-11 Thread edan73 
You make very valid points. I will consider using the -isystem flag. However i 
would like to point out that i am not expecting protobuf itself to compile with 
all the same warnings strictures that my code does - indeed that would be 
unreasonable. But i think providing warning-free headers is rather more 
reasonable to expect, and indeed comparable to the generated code situation 
that you mention, since i must obviously compile the headers with my project's 
flags. Treating them as system headers should not really be needed, as after 
all they are not in fact. Thoughts?

-original message-
Subject: Re: [protobuf] Re: 2.3.0 released
From: Kenton Varda ken...@google.com
Date: 11.01.2010 20.35

We get a lot of complaints about warnings in our code.  When the warnings
occur in generated code, I generally fix them (because generated code
generally must be compiled using your project's flags), but for warnings in
protobuf code my answer is that you need to include the protobuf headers as
system headers so that warnings are ignored.  Different projects make
different choices about what warnings to enable on their code, so it seems
unreasonable to expect that every one of your dependencies enable at least
all of the warnings that you enable.  The sign comparison warnings are
particularly annoying because they warn about every line that looks like:
  for (int i = 0; i  some_vector.size(); i++)
which are very common and perfectly valid.  Therefore we compile with
-fno-sign-compare.

That said, this patch seems harmless, so I'll submit it.  But I won't be
doing a new release just for this, and new warnings could easily be
introduced before the next release.

On Sun, Jan 10, 2010 at 3:13 AM, edan eda...@gmail.com wrote:

 I looks like a good workaround - thanks for the info.
 I will wait and see if Kenton is planning to fix this, then decide my next
 steps.
 --edan


 On Sun, Jan 10, 2010 at 10:59 AM, Monty Taylor mord...@inaugust.comwrote:



 edan wrote:
  I happily upgraded to 2.3.0 - I always like to take the latest and
 greatest.
  Unfortunately, and I think for the first time ever while upgrading
  protobuf, I ran into a problem!
  We compile our code with -Werror, and this bombed out on a header
 file:

 We build with errors on in our project too - our solution to this has
 been to:

 a) install the headers into a system locatiom, at which point gcc will
 not issue warnings for them. It looks like you did this in the context
 of your /devfs location - perhaps you need to change some system configs
 to completely understand that location as a chroot?

 b) If they aren't in a system location, include them via -isystem rather
 than -I, which will have the same effect.

  cc1plus: warnings being treated as errors
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian32(google::protobuf::uint32*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:776:
  warning: comparison between signed and unsigned integer expressions
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian64(google::protobuf::uint64*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:791:
  warning: comparison between signed and unsigned integer expressions
 
 
  My patch to fix this was:
 
  
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#2
  (ktext) -
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#3
  (ktext)  content
  776c776
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
  791c791
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
 
  Any chance you can patch this and re-release?  I'd really like to have
  un-patched code in our product, but I can't use 2.3.0 without this
 patch.




-- 
You received this message because you are subscribed to the Google Groups 
Protocol Buffers group.
To post to this group, send email to proto...@googlegroups.com.
To unsubscribe from this group, send email to 
protobuf+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-11 Thread Christopher Smith 
I hate to quibble on this, but strictly speaking:

 for (int i = 0; i  some_vector.size(); i++)

is not perfectly valid unless you have verified that some_vector.size() 
static_castsize_t(std::numeric_limitsint::max());

This would be broken in cases of a very large vector (possible with a
vectorunsigned char on 32-bit Linux or that abomination vectorbool
almost anywhere), or if you had a platform with sizeof(int) 
sizeof(size_t), which is surprisingly common (for example, on Solaris:
http://docsun.cites.uiuc.edu/sun_docs/C/solaris_9/SUNWdev/SOL64TRANS/p6.html#CHAPTER3-TBL-3
).

Sometimes, compilers warn about really dangerous things.

--Chris

On Mon, Jan 11, 2010 at 10:35 AM, Kenton Varda ken...@google.com wrote:

 We get a lot of complaints about warnings in our code.  When the warnings
 occur in generated code, I generally fix them (because generated code
 generally must be compiled using your project's flags), but for warnings in
 protobuf code my answer is that you need to include the protobuf headers as
 system headers so that warnings are ignored.  Different projects make
 different choices about what warnings to enable on their code, so it seems
 unreasonable to expect that every one of your dependencies enable at least
 all of the warnings that you enable.  The sign comparison warnings are
 particularly annoying because they warn about every line that looks like:
   for (int i = 0; i  some_vector.size(); i++)
 which are very common and perfectly valid.  Therefore we compile with
 -fno-sign-compare.

 That said, this patch seems harmless, so I'll submit it.  But I won't be
 doing a new release just for this, and new warnings could easily be
 introduced before the next release.


 On Sun, Jan 10, 2010 at 3:13 AM, edan eda...@gmail.com wrote:

 I looks like a good workaround - thanks for the info.
 I will wait and see if Kenton is planning to fix this, then decide my next
 steps.
 --edan


 On Sun, Jan 10, 2010 at 10:59 AM, Monty Taylor mord...@inaugust.comwrote:



 edan wrote:
  I happily upgraded to 2.3.0 - I always like to take the latest and
 greatest.
  Unfortunately, and I think for the first time ever while upgrading
  protobuf, I ran into a problem!
  We compile our code with -Werror, and this bombed out on a header
 file:

 We build with errors on in our project too - our solution to this has
 been to:

 a) install the headers into a system locatiom, at which point gcc will
 not issue warnings for them. It looks like you did this in the context
 of your /devfs location - perhaps you need to change some system configs
 to completely understand that location as a chroot?

 b) If they aren't in a system location, include them via -isystem rather
 than -I, which will have the same effect.

  cc1plus: warnings being treated as errors
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian32(google::protobuf::uint32*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:776:
  warning: comparison between signed and unsigned integer expressions
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian64(google::protobuf::uint64*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:791:
  warning: comparison between signed and unsigned integer expressions
 
 
  My patch to fix this was:
 
  
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#2
  (ktext) -
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#3
  (ktext)  content
  776c776
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
  791c791
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
 
  Any chance you can patch this and re-release?  I'd really like to have
  un-patched code in our product, but I can't use 2.3.0 without this
 patch.




 --
 You received this message because you are subscribed to the Google Groups
 Protocol Buffers group.
 To post to this group, send email to proto...@googlegroups.com.
 To unsubscribe from this group, send email to
 protobuf+unsubscr...@googlegroups.comprotobuf%2bunsubscr...@googlegroups.com
 .
 For more options, visit this group at
 http://groups.google.com/group/protobuf?hl=en.




-- 
Chris
-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-11 Thread Kenton Varda 
On Mon, Jan 11, 2010 at 11:02 AM, eda...@gmail.com wrote:

 You make very valid points. I will consider using the -isystem flag.
 However i would like to point out that i am not expecting protobuf itself to
 compile with all the same warnings strictures that my code does - indeed
 that would be unreasonable. But i think providing warning-free headers is
 rather more reasonable to expect, and indeed comparable to the generated
 code situation that you mention, since i must obviously compile the headers
 with my project's flags. Treating them as system headers should not really
 be needed, as after all they are not in fact. Thoughts?


It's certainly more-reasonable, but in C++ where inlined source code
commonly appears in headers, it's still quite hard to accomplish this.  Is
there a GCC flag which would allow me to compile the protobuf package itself
with different warning settings for headers vs. source files?
-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

[protobuf] Re: 2.3.0 released

2010-01-11 Thread Kenton Varda 
Documentation has been updated.

On Fri, Jan 8, 2010 at 4:51 PM, Kenton Varda ken...@google.com wrote:

 I've pushed the final release of Protobuf 2.3.0:

 http://code.google.com/p/protobuf/downloads/list

 Documentation updates are still in review but I hope to have them up
 Monday.

 2009-01-08 version 2.3.0:

   General
   * Parsers for repeated numeric fields now always accept both packed and
 unpacked input.  The [packed=true] option only affects serializers.
 Therefore, it is possible to switch a field to packed format without
 breaking backwards-compatibility -- as long as all parties are using
 protobuf 2.3.0 or above, at least.
   * The generic RPC service code generated by the C++, Java, and Python
 generators can be disabled via file options:
   option cc_generic_services = false;
   option java_generic_services = false;
   option py_generic_services = false;
 This allows plugins to generate alternative code, possibly specific to 
 some
 particular RPC implementation.

   protoc
   * Now supports a plugin system for code generators.  Plugins can generate
 code for new languages or inject additional code into the output of other
 code generators.  Plugins are just binaries which accept a protocol buffer
 on stdin and write a protocol buffer to stdout, so they may be written in
 any language.  See src/google/protobuf/compiler/plugin.proto.
 **WARNING**:  Plugins are experimental.  The interface may change in a
 future version.
   * If the output location ends in .zip or .jar, protoc will write its output
 to a zip/jar archive instead of a directory.  For example:
   protoc --java_out=myproto_srcs.jar --python_out=myproto.zip 
 myproto.proto
 Currently the archive contents are not compressed, though this could 
 change
 in the future.
   * inf, -inf, and nan can now be used as default values for float and double
 fields.

   C++
   * Various speed and code size optimizations.
   * DynamicMessageFactory is now fully thread-safe.
   * Message::Utf8DebugString() method is like DebugString() but avoids 
 escaping
 UTF-8 bytes.
   * Compiled-in message types can now contain dynamic extensions, through use
 of CodedInputStream::SetExtensionRegistry().
   * Now compiles shared libraries (DLLs) by default on Cygwin and MinGW, to
 match other platforms.  Use --disable-shared to avoid this.

   Java
   * parseDelimitedFrom() and mergeDelimitedFrom() now detect EOF and return
 false/null instead of throwing an exception.
   * Fixed some initialization ordering bugs.
   * Fixes for OpenJDK 7.

   Python
   * 10-25 times faster than 2.2.0, still pure-Python.
   * Calling a mutating method on a sub-message always instantiates the message
 in its parent even if the mutating method doesn't actually mutate anything
 (e.g. parsing from an empty string).
   * Expanded descriptors a bit.



-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-11 Thread edan 
 Is there a GCC flag which would allow me to compile the protobuf package
 itself with different warning settings for headers vs. source files?


No, I can't imagine there is.  But what you could do is create a unit test
that runs during make check whose sole purpose is to include all the
headers, and compile with -Werror.  Then you would catch any warnings in
the headers and be able to fix them before release.  Would this work, and is
it something you'd consider doing?
-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-11 Thread Kenton Varda 
On Mon, Jan 11, 2010 at 10:45 PM, Kenton Varda ken...@google.com wrote:

 On Mon, Jan 11, 2010 at 10:08 PM, edan eda...@gmail.com wrote:

 No, I can't imagine there is.  But what you could do is create a unit test
 that runs during make check whose sole purpose is to include all the
 headers, and compile with -Werror.  Then you would catch any warnings in
 the headers and be able to fix them before release.  Would this work, and is
 it something you'd consider doing?


 Hmm, yes, that sounds quite reasonable.  Of course, it will only cover
 GCC's warnings, but that should take care of about half the complaints we
 get.  Can you write up a patch for this?


Though there is the problem that if the test is run on a compiler with
stricter warnings, it could fail to compile, which could unnecessarily block
the running of any other tests, which is annoying.  At the very least we'd
have to limit the test to GCC platforms only since there are too many other
compilers out there.  Maybe this really needs to be separate from the
protobuf package, but something we run as part of the release process...
-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-10 Thread Monty Taylor 


edan wrote:
 I happily upgraded to 2.3.0 - I always like to take the latest and greatest.
 Unfortunately, and I think for the first time ever while upgrading
 protobuf, I ran into a problem!
 We compile our code with -Werror, and this bombed out on a header file:

We build with errors on in our project too - our solution to this has
been to:

a) install the headers into a system locatiom, at which point gcc will
not issue warnings for them. It looks like you did this in the context
of your /devfs location - perhaps you need to change some system configs
to completely understand that location as a chroot?

b) If they aren't in a system location, include them via -isystem rather
than -I, which will have the same effect.

 cc1plus: warnings being treated as errors
 ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
 function bool
 google::protobuf::io::CodedInputStream::ReadLittleEndian32(google::protobuf::uint32*):
 ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:776:
 warning: comparison between signed and unsigned integer expressions
 ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
 function bool
 google::protobuf::io::CodedInputStream::ReadLittleEndian64(google::protobuf::uint64*):
 ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:791:
 warning: comparison between signed and unsigned integer expressions
 
 
 My patch to fix this was:
 
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#2
 (ktext) -
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#3
 (ktext)  content
 776c776
if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
 ---
   if (GOOGLE_PREDICT_TRUE(BufferSize() =
 static_castint(sizeof(*value {
 791c791
if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
 ---
   if (GOOGLE_PREDICT_TRUE(BufferSize() =
 static_castint(sizeof(*value {
 
 Any chance you can patch this and re-release?  I'd really like to have
 un-patched code in our product, but I can't use 2.3.0 without this patch.
-- 
You received this message because you are subscribed to the Google Groups 
Protocol Buffers group.
To post to this group, send email to proto...@googlegroups.com.
To unsubscribe from this group, send email to 
protobuf+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/protobuf?hl=en.

Re: [protobuf] Re: 2.3.0 released

2010-01-10 Thread edan 
I looks like a good workaround - thanks for the info.
I will wait and see if Kenton is planning to fix this, then decide my next
steps.
--edan

On Sun, Jan 10, 2010 at 10:59 AM, Monty Taylor mord...@inaugust.com wrote:



 edan wrote:
  I happily upgraded to 2.3.0 - I always like to take the latest and
 greatest.
  Unfortunately, and I think for the first time ever while upgrading
  protobuf, I ran into a problem!
  We compile our code with -Werror, and this bombed out on a header file:

 We build with errors on in our project too - our solution to this has
 been to:

 a) install the headers into a system locatiom, at which point gcc will
 not issue warnings for them. It looks like you did this in the context
 of your /devfs location - perhaps you need to change some system configs
 to completely understand that location as a chroot?

 b) If they aren't in a system location, include them via -isystem rather
 than -I, which will have the same effect.

  cc1plus: warnings being treated as errors
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian32(google::protobuf::uint32*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:776:
  warning: comparison between signed and unsigned integer expressions
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h: In member
  function bool
 
 google::protobuf::io::CodedInputStream::ReadLittleEndian64(google::protobuf::uint64*):
  ../../../devfs/usr/include/google/protobuf/io/coded_stream.h:791:
  warning: comparison between signed and unsigned integer expressions
 
 
  My patch to fix this was:
 
  
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#2
  (ktext) -
 
 //depot/project/zenith/ports/protobuf/std/build/src/google/protobuf/io/coded_stream.h#3
  (ktext)  content
  776c776
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
  791c791
 if (GOOGLE_PREDICT_TRUE(BufferSize() = sizeof(*value))) {
  ---
if (GOOGLE_PREDICT_TRUE(BufferSize() =
  static_castint(sizeof(*value {
 
  Any chance you can patch this and re-release?  I'd really like to have
  un-patched code in our product, but I can't use 2.3.0 without this patch.

-- 

You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.

To post to this group, send email to proto...@googlegroups.com.

To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.