search

exported / snappy

Automatically exported from code.google.com/p/snappy
Other
0 stars 0 forks source link

Various MSVC x64 compiler size_t warnings (C4267) #75

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Seen when compiling snappy for inclusion into Chromium:

e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(205) : error 
C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(205) : warning 
C4267: '=' : conversion from 'size_t' to 'char', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(209) : warning 
C4267: 'argument' : conversion from 'size_t' to 'snappy::uint16', possible loss 
of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(267) : warning 
C4267: '=' : conversion from 'size_t' to 'int', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(789) : warning 
C4267: '=' : conversion from 'size_t' to 'snappy::uint32', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(818) : warning 
C4267: 'argument' : conversion from 'size_t' to 'const snappy::uint32', 
possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(879) : warning 
C4267: 'argument' : conversion from 'size_t' to 'snappy::uint32', possible loss 
of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(929) : warning 
C4267: 'initializing' : conversion from 'size_t' to 'const int', possible loss 
of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(1021) : warning 
C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(1026) : warning 
C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(752) : warning 
C4267: '=' : conversion from 'size_t' to 'snappy::uint32', possible loss of data
e:\b\build\slave\win\build\src\third_party\snappy\src\snappy.cc(752) : warning 
C4267: '=' : conversion from 'size_t' to 'snappy::uint32', possible loss of data

Original issue reported on code.google.com by jsbell@chromium.org on 29 Apr 2013 at 5:52

GoogleCodeExporter commented 9 years ago 
I can confirm this. (MSVC 9.0 SP1, x64)

Original comment by stream.n...@gmx.de on 8 May 2013 at 9:20

GoogleCodeExporter commented 9 years ago 
Forgot to add: Happens with Snappy 1.1.0

Original comment by stream.n...@gmx.de on 8 May 2013 at 9:22

GoogleCodeExporter commented 9 years ago 
I've looked a bit of these.

Note that in general, Snappy is not meant to support compressing blocks of 
64-bit size, so in itself, this is not a very high-priority goal.

We have a few classes of these warnings: One is the kind where we knowingly 
narrow the size_t into something much smaller. See e.g. this example from 
EmitCopyLessThan64, with len_minus_4 and offset being size_t, and op being 
char*:

  *op++ = COPY_1_BYTE_OFFSET + ((len_minus_4) << 2) + ((offset >> 8) << 5);
  *op++ = offset & 0xff;

We already have ample asserts here, so there's no bug that the warning 
uncovers. To fix it, we could have to do something like

  *op++ = static_cast<uint8>(COPY_1_BYTE_OFFSET + ((len_minus_4) << 2) + ((offset >> 8) << 5));
  *op++ = static_cast<uint8>(offset & 0xff);

except we'd need to break the line:

  *op++ = static_cast<uint8>(COPY_1_BYTE_OFFSET + ((len_minus_4) << 2) +
                             ((offset >> 8) << 5));
  *op++ = static_cast<uint8>(offset & 0xff);

I think this reduces readability enough that it's a clear negative.

Another, more interesting class is in cases like this:

  static inline void IncrementalCopyFastPath(const char* src, char* op, int len) {

where IncrementalCopyFastPath is called with a size_t argument for len. Also, 
we repeatedly do things like:

    len -= op - src;

We can't have len be size_t since we need to be able to check for it being 
negative; however, it could reasonably be ssize_t. As an added bonus, it would 
then seem we can avoid a signed 32-to-64-bit conversion on a hot path, which 
wins us something like 2% in decompression. It looks like this varies a bit 
between GCC versions, though; it's easy to get unlucky with what's presumably a 
good change.

Unfortunately, just changing everything blindly to ssize_t where it used to be 
int doesn't really help; seemingly, the cases here have to be implemented and 
benchmarked on a case-by-case basis, since some of them appears to hurt 
performance (for whatever reason).

So, in short, while some of these are interesting, I don't think we can fix all 
of them without unduly hurting readability and/or speed, so in that case, I'd 
say you should simply disable the warning in Chromium.

Comments?

Original comment by se...@google.com on 14 Jun 2013 at 2:52

GoogleCodeExporter commented 9 years ago 
Yep, we've already disabled the warning in Chromium. Thanks for taking a look.

Original comment by jsbell@chromium.org on 14 Jun 2013 at 5:02

GoogleCodeExporter commented 9 years ago 
r77 is relevant for this bug.

I'll leave it open until I get to look if the other cases have performance 
impact, just so I have a tracker on that. After that, I'll close it, since 
we're not going to be fixing all of them.

Original comment by se...@google.com on 14 Jun 2013 at 9:44

GoogleCodeExporter commented 9 years ago 
Found no further ones. Closing as mentioned.

Original comment by se...@google.com on 1 Jul 2013 at 11:03