GCC build warnings

[GoogleCodeExporter]

What is the expected output? What do you see instead?
When building snappy as part of Firefox, I get the following warnings from GCC:

snappy.cc: In member function ‘snappy::uint16* 
snappy::internal::WorkingMemory::GetHashTable(size_t, int*)’:
snappy.cc:253:49: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:260:17: warning: comparison between signed and unsigned integer 
expressions
snappy.cc: In function ‘char* snappy::internal::CompressFragment(const char*, 
size_t, char*, snappy::uint16*, int)’:
snappy.cc:304:3: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:315:7: warning: comparison between signed and unsigned integer 
expressions
snappy.cc: At global scope:
snappy.cc:504:69: warning: extra ‘;’
snappy.cc:590:52: warning: extra ‘;’
snappy.cc: In function ‘size_t snappy::Compress(snappy::Source*, 
snappy::Sink*)’:
snappy.cc:818:23: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:833:27: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:840:7: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:844:5: warning: comparison between signed and unsigned integer 
expressions
snappy.cc: In member function ‘bool snappy::SnappyArrayWriter::Append(const 
char*, snappy::uint32, bool)’:
snappy.cc:913:24: warning: comparison between signed and unsigned integer 
expressions
snappy.cc: In member function ‘bool 
snappy::SnappyArrayWriter::AppendFromSelf(snappy::uint32, snappy::uint32)’:
snappy.cc:934:31: warning: comparison between signed and unsigned integer 
expressions
snappy.cc:937:26: warning: comparison between signed and unsigned integer 
expressions
snappy.cc: At global scope:
snappy.cc:516:13: warning: ‘void snappy::ComputeTable()’ defined but not 
used

Patch attached to fix them.

Original issue reported on code.google.com by Ms2...@gmail.com on 20 Dec 2011 at 2:42

Attachments:

• snappy.diff

[GoogleCodeExporter]

Hi,

The signed/unsigned warnings are an unfortunate side effect of Google's C++ 
coding style, and we disable those internally, so I don't think we can uglify 
the code with casts everywhere to that effect. Of course, we also can't put 
#ifdef GOOGLE around things just because they cause warnings in Firefox :-)

We can fix the extra semicolons in DECLARE_bool, but those fixes need to be in 
snappy-stubs-internal.h (they're a reimplementation of an internal API). 
REGISTER_MODULE_INITIALIZER is a similar story, but without an obvious fix; I 
can see if I can get that line out of the open-source distribution somehow.

Original comment by se...@google.com on 20 Dec 2011 at 2:48

[GoogleCodeExporter]

The ifdef GOOGLE part is because those static functions are unused in the open 
source version, so it seems silly to keep building it.

Original comment by Ms2...@gmail.com on 20 Dec 2011 at 3:04

[GoogleCodeExporter]

Yeah, they are unused in the open source version, in the sense that they're not 
used in a default build. They're still intended to be useful for reconstructing 
the table, though.

Original comment by se...@google.com on 20 Dec 2011 at 4:09

[GoogleCodeExporter]

In other code that I've opensourced from google (e.g. google-sparsehash, 
google-ctemplate), I've rewritten the code to use size_t and other appropriate 
unsigned variables, rather than int, even when int is the 
google-style-preferred format.  (Of course, I carefully audit each change to 
make sure the number really can't be negative, and cause any weird promotion 
issues!)  To my mind, this is one of the places where two style rules conflict 
-- use int type, have no warnings -- and while we can resolve the conflict by 
adding -Wno-unsgined-compared to the snappy build, I think a better fix is to 
just use unsigned types here.

Looking at the actual patch, I think an unsigned type is called for in any case 
with htsize, which is doing <<= which is fragile for signed types (inside 
google, we'd probably ask that it be an int64), and is safe enough for 
kBlockSize, kInputMarginBytes, etc.  I do agree that the fix should be to move 
appropriate types from signed to unsigned, and not to throw in casts 
everywhere.  That said, I would also agree it's appropriate to just leave the 
code as it is, and maybe suppress the warning in the Makefile.

If you decide to do the latter, here's how I do it, in case that helps:
   In configure.ac:
     AM_CONDITIONAL(GCC, test "$GCC" = yes)   # let the Makefile know if we're gcc
   In Makefile.am:
     if GCC
     AM_CXXFLAGS += -Wall -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare
     endif GCC

Original comment by csilv...@gmail.com on 20 Dec 2011 at 11:19

[GoogleCodeExporter]

I took a shot at fixing most of the warnings by using size_t for lengths etc. 
(with the added bonus that maybe we can encode things larger than 2^32 bytes in 
one go if someone would be crazy enough).

Unfortunately, the changes make for a performance loss of a few percent in 
decompression. I'll need to figure out what's going on, but now I'll be going 
on vacation. :-) I'll try to pick it up sometime in January.

Original comment by se...@google.com on 22 Dec 2011 at 3:08

• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Fixed in r56.

Original comment by se...@google.com on 4 Jan 2012 at 1:11

• Changed state: Fixed

[GoogleCodeExporter]

Issue 58 has been merged into this issue.

Original comment by se...@google.com on 4 Jan 2012 at 4:46