Issue 361 in protobuf: Fix warnings

[prot...@googlecode.com]

Status: New
Owner: liuj...@google.com
Labels: Type-Defect Priority-Medium

New issue 361 by NN14...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

VC2008 with level warning 4 produces several warnings:

1>google\protobuf\stubs\common.h(1201) : warning
C4512: 'google::protobuf::FatalException' : assignment operator could not
be generated

1>google\protobuf\io\coded_stream.h(901) : warning C4244: '=' : conversion
from 'google::protobuf::uint32' to google::protobuf::uint8', possible loss
of data

1>google\protobuf\io\coded_stream.h(965) : warning C4244: '=' : conversion
from 'google::protobuf::uint32' to google::protobuf::uint8', possible loss
of data

Fix it please.

[prot...@googlecode.com]

Comment #1 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

gcc also generates a lot of errors "comparison between signed and unsigned
integer expressions" if protobuf is compiled with flags "-Wall -w9 -Werror"

[prot...@googlecode.com]

Comment #2 on issue 361 by jacobgla...@yahoo.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

There's also a bunch of size mismatches that I'd love to see fixed.

[prot...@googlecode.com]

Comment #3 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

There are also few warnings I don't know how to handle properly

protobuf-2.4.1\src\google\protobuf\text_format.cc:126: warning:
C4355: 'this' : used in base member initializer list
protobuf-2.4.1\src\google\protobuf\compiler\importer.cc:118: warning:
C4355: 'this' : used in base member initializer list

protobuf-2.4.1\src\google\protobuf\text_format.cc:423: warning:
C4800: 'google::protobuf::uint64' : forcing value to bool 'true' or 'false'
(performance warning)
protobuf-2.4.1\src\google\protobuf\text_format.cc:423: warning:
C4800: 'google::protobuf::uint64' : forcing value to bool 'true' or 'false'
(performance warning)
protobuf-2.4.1\src\google\protobuf\generated_message_reflection.cc:1149:
warning: C4800: 'const google::protobuf::uint32' : forcing value to
bool 'true' or 'false' (performance warning)

[prot...@googlecode.com]

Comment #4 on issue 361 by NN14...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

C4800: 'google::protobuf::uint64' : forcing value to bool 'true' or 'false'
This is simple write the correct expression.

Generates warning:
uint64 x = 1;
bool a = x;

No warning, and makes code more readable.
uint64 x = 1;
bool a = x != 0;

About this in base member initializer list, it can be problem and can be
false alarm.
It depends.

Problematic code:
// Some interface
class I { virtual void Do() = 0; };

class A { public: A(I& i) { i.Do(); } };
class B : public A { public: B() : A(*this) { } } // BOOM !

Non problematic code, in case we just store reference/pointer.
// Some interface
class I { virtual void Do() = 0; };

class A { public: A(I& i) : _i(i) { } private: I& _i; };
class B : public A { public: B() : A(*this) { } } // No problems.

If it is the second case, disable warning by

#pragma warning(push)
#pragma warning(disable: 4355)

... Code

#pragma warning(pop)

Of course it is for MSVC, I don't know if GCC complains about it.

[prot...@googlecode.com]

Updates:
Status: Accepted

Comment #5 on issue 361 by liuj...@google.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

(No comment was entered for this change.)

[prot...@googlecode.com]

Comment #6 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

Thanks.

What about this:
src\google\protobuf\stubs\common_unittest.cc:75: warning: C4307: '+' :
integral constant overflow

EXPECT_EQ(0u, kuint32max + 1);

Seems like this is done intentionally. Should it also be supressed with
pragma?

[prot...@googlecode.com]

Comment #7 on issue 361 by tom.ritc...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

In my setup, at least, GCC doesn't complain about "forcing value to bool",
or about "'this' : used in base member initializer list".

I frankly think the "forcing value to bool" is a bogus error that should be
suppressed in everyone's project. I honestly don't believe there is any
significant "performance" penalty at all - the last time I looked at the
generated code for bool a = x; and bool a = (x != 0); they were exactly the
same.

The unit test warning probably should be suppressed with a pragma BUT if it
isn't bothering people within Google IMHO this is a much lower priority.

As an external developer who makes heavy use of protocol buffers, I only
ever ran the Google unit tests one time, and I really didn't care if there
were a few warnings there. But warnings in my main build are troublesome
because I like to have none at all so I can clearly see "real" warnings
when they come up.

[prot...@googlecode.com]

Comment #8 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

I've fixed all warnings in protobuf itself, only warnings for unittests
left:

google/protobuf/compiler/cpp/cpp_unittest.cc:878:5: ошибка: case value
«12589235» not in enumerated type «protobuf_unittest::TestSparseEnum»
[-Werror=switch]

[prot...@googlecode.com]

Comment #9 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

This patch is for protobuf version 2.4.1

There several types of warnings left: unused-variable, conversion-null,
overflow, switch

All signed versus unsinged warnings fixed (using explicit static_cast).

Attachments:
fix_warnings_2.4.1.patch 151 KB

[prot...@googlecode.com]

Comment #10 on issue 361 by zabulu...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

This patch fixes issues topic starter posted about

Attachments:
1.patch 1.8 KB

[prot...@googlecode.com]

Comment #11 on issue 361 by xiaof...@google.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

"int" is used as the loop counter intentionally. We would rather disable
the warning instead of using "size_t".

[prot...@googlecode.com]

Comment #12 on issue 361 by Van...@gmail.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

I don't see the reason why? If size() returns size_t, why then i should be
int it the following statement:

for (i = 0; i < input_files_.size(); i++)

[prot...@googlecode.com]

Comment #13 on issue 361 by xiaof...@google.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

It's the convention at Google to use "int" as loop counters, which makes it
pointless to update the code to use "size_t" as it's inevitable that new
code will introduce more "int"s in the future.
Unless Google's style guide discourages the use of "int", we shouldn't
bother to make the change.

[prot...@googlecode.com]

Comment #14 on issue 361 by jacobgla...@yahoo.com: Fix warnings
http://code.google.com/p/protobuf/issues/detail?id=361

How about something like:

const int n = static_cast<int>(input_files_.size());
for (i = 0; i < n; i++)

[prot...@googlecode.com]

Updates:
Status: WontFix
Owner: xiaof...@google.com

Comment #15 on issue 361 by xiaof...@google.com: Fix warnings
https://code.google.com/p/protobuf/issues/detail?id=361

Closing this as "WontFix".

If you still get warnings with the newly released 2.6.0, please help file
an issue or send us a patch on our new github site:
https://github.com/google/protobuf

Note that for the signed/unsigned comparison warning, we won't change our
source code to avoid that warning. Instead, I think the right approach is
to add "pragma" preprocessors to suppress this warning on certain compilers.

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[prot...@googlecode.com]

Comment #16 on issue 361 by dholb...@gmail.com: Fix warnings
https://code.google.com/p/protobuf/issues/detail?id=361

RE:

> It's the convention at Google to use "int" as
> loop counters, which makes it pointless to
> update the code to use "size_t"

Surely the max-value that the loop counter is compared against should
*also* have type "int", too, then? (as suggested in comment #14)

Signed/unsigned comparisons can cause subtle bugs and are worth avoiding,
one way or another...