Fixing level 4 warnings

Treat warnings as error and fix most level 4 warnings in the breakpad windows
client projects.

Some of the lint errors in the files touched by this change were also fixed.

BUG=533
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1189

[Ivan Penkov]

.

[Mark Mentovai]

Not LGTM.

Warnings complaining about unused parameters and |while (true)| are not useful.
We don’t use these warnings in Chrome. They force you to litter code with
unnecessary annotations and less-readable forms.

If you want to use this warning level, please disable the useless warnings.

[Ivan Penkov]

On 2013/05/30 12:35:55, Mark Mentovai wrote:
> Not LGTM.
> 
> Warnings complaining about unused parameters and |while (true)| are not
useful.
> We don’t use these warnings in Chrome. They force you to litter code with
> unnecessary annotations and less-readable forms.
> 
> If you want to use this warning level, please disable the useless warnings.

Done.

[Mark Mentovai]

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
File src/client/windows/build/common.gypi (right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
src/client/windows/build/common.gypi:1: # Copyright (c) 2010, Google Inc.
In the CL description, instead of the last line of prose, you can write

BUG=533

That’s the standard form for referencing a bug.

I’m not sure if that bug still applies, though, or if it should be closed as
WontFix.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
src/client/windows/build/common.gypi:1177: 'msvs_disabled_warnings': [4396,
4503, 4819, 4995, 4512, 4127, 4100],
This list was sorted before, you should keep it sorted.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/handler/exc...
File src/client/windows/handler/exception_handler.cc (right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/handler/exc...
src/client/windows/handler/exception_handler.cc:389: for (;;) {
Leave this as while (true).

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/tests/crash...
File src/client/windows/tests/crash_generation_app/crash_generation_app.cc
(right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/tests/crash...
src/client/windows/tests/crash_generation_app/crash_generation_app.cc:186:
UNREFERENCED_PARAMETER(assertion);
I thought you were going to get rid of these.

[Ivan Penkov]

.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
File src/client/windows/build/common.gypi (right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
src/client/windows/build/common.gypi:1: # Copyright (c) 2010, Google Inc.
On 2013/05/30 19:06:44, Mark Mentovai wrote:
> In the CL description, instead of the last line of prose, you can write
> 
> BUG=533
> 
> That’s the standard form for referencing a bug.
> 
> I’m not sure if that bug still applies, though, or if it should be closed as
> WontFix.

Done.  The bug still applies.  I ended up removing 2 unused parameters which
will resolve the "unused parameter" warning.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/build/commo...
src/client/windows/build/common.gypi:1177: 'msvs_disabled_warnings': [4396,
4503, 4819, 4995, 4512, 4127, 4100],
On 2013/05/30 19:06:44, Mark Mentovai wrote:
> This list was sorted before, you should keep it sorted.

Done.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/handler/exc...
File src/client/windows/handler/exception_handler.cc (right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/handler/exc...
src/client/windows/handler/exception_handler.cc:389: for (;;) {
On 2013/05/30 19:06:44, Mark Mentovai wrote:
> Leave this as while (true).

Bummer.  I did fix those, but I did the change through meld and apparently it
failed to overwrite the original file. Sorry.  It is done now for real.

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/tests/crash...
File src/client/windows/tests/crash_generation_app/crash_generation_app.cc
(right):

https://breakpad.appspot.com/601002/diff/13001/src/client/windows/tests/crash...
src/client/windows/tests/crash_generation_app/crash_generation_app.cc:186:
UNREFERENCED_PARAMETER(assertion);
On 2013/05/30 19:06:44, Mark Mentovai wrote:
> I thought you were going to get rid of these.

Same here.  Done.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/handler/exc...
File src/client/windows/handler/exception_handler.h (right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/handler/exc...
src/client/windows/handler/exception_handler.h:66: #pragma warning(disable :
4530)
You seem to be tweaking these to make them consistent with each other, but this
one has spaces around the colon…

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/tests/crash...
File src/client/windows/tests/crash_generation_app/crash_generation_app.cc
(right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/tests/crash...
src/client/windows/tests/crash_generation_app/crash_generation_app.cc:361:
#pragma warning(disable:4312)
…and this one doesn’t.

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/unittests/m...
File src/client/windows/unittests/minidump_test.cc (right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/unittests/m...
src/client/windows/unittests/minidump_test.cc:165: ::ImagehlpApiVersion();
:: is unconventional in our code except where needed to disambiguate.

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload.cc
File src/common/windows/http_upload.cc (right):

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload...
src/common/windows/http_upload.cc:33: #pragma warning(disable : 4530)
Per my previous comment, consider the spacing around the colon.

[Mark Mentovai]

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload.cc
File src/common/windows/http_upload.cc (right):

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload...
src/common/windows/http_upload.cc:340: unsigned int uint_length =
static_cast<unsigned int>(length);
What are you trying to avoid overflowing? What’s special about unsigned int
here? A vector is indexed by its size_type (size_t). size_t might have a
different range than unsigned int.

[Ivan Penkov]

.

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/handler/exc...
File src/client/windows/handler/exception_handler.h (right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/handler/exc...
src/client/windows/handler/exception_handler.h:66: #pragma warning(disable :
4530)
On 2013/05/31 03:48:52, Mark Mentovai wrote:
> You seem to be tweaking these to make them consistent with each other, but
this
> one has spaces around the colon…

Done.

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/tests/crash...
File src/client/windows/tests/crash_generation_app/crash_generation_app.cc
(right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/tests/crash...
src/client/windows/tests/crash_generation_app/crash_generation_app.cc:361:
#pragma warning(disable:4312)
On 2013/05/31 03:48:52, Mark Mentovai wrote:
> …and this one doesn’t.

I removed the spacing on the other one.

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/unittests/m...
File src/client/windows/unittests/minidump_test.cc (right):

https://breakpad.appspot.com/601002/diff/13002/src/client/windows/unittests/m...
src/client/windows/unittests/minidump_test.cc:165: ::ImagehlpApiVersion();
On 2013/05/31 03:48:52, Mark Mentovai wrote:
> :: is unconventional in our code except where needed to disambiguate.

Done.

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload.cc
File src/common/windows/http_upload.cc (right):

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload...
src/common/windows/http_upload.cc:33: #pragma warning(disable : 4530)
On 2013/05/31 03:48:52, Mark Mentovai wrote:
> Per my previous comment, consider the spacing around the colon.

Done.

https://breakpad.appspot.com/601002/diff/13002/src/common/windows/http_upload...
src/common/windows/http_upload.cc:340: unsigned int uint_length =
static_cast<unsigned int>(length);
On 2013/05/31 04:12:50, Mark Mentovai wrote:
> What are you trying to avoid overflowing? What’s special about unsigned int
> here? A vector is indexed by its size_type (size_t). size_t might have a
> different range than unsigned int.

The issue here is that file.tellg() returns a signed 64-bit int which is then
fed into std::vector<char>::resize() which only accepts unsigned ints (uint_32).
 Hence, there is the potential for data loss when the stream size is greater
than MAX_UINT_32.

However, instead of using directly "unsigned int", I should have used the actual
size type defined in std::vector.

Done.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/601002/diff/25001/src/common/windows/http_upload.cc
File src/common/windows/http_upload.cc (right):

https://breakpad.appspot.com/601002/diff/25001/src/common/windows/http_upload...
src/common/windows/http_upload.cc:343: if
(static_cast<std::streamoff>(vector_size) == length) {
I thought that both size_type and streamoff were “long” (32-bit) on 32-bit
Windows, and “long long” or some other 64-bit type on 64-bit windows. The
difference is that size_type is unsigned and streamoff is signed. Is this
correct?

If so, wouldn’t a more straightforward way to check that your streamoff length
can fit in a size_type be to just make sure that length >= 0?

[Ivan Penkov]

.

https://breakpad.appspot.com/601002/diff/25001/src/common/windows/http_upload.cc
File src/common/windows/http_upload.cc (right):

https://breakpad.appspot.com/601002/diff/25001/src/common/windows/http_upload...
src/common/windows/http_upload.cc:343: if
(static_cast<std::streamoff>(vector_size) == length) {
On 2013/06/04 13:19:41, Mark Mentovai wrote:
> I thought that both size_type and streamoff were “long”&nbsp;(32-bit) on
32-bit
> Windows, and “long long” or some other 64-bit type on 64-bit windows. The
> difference is that size_type is unsigned and streamoff is signed. Is this
> correct?
> 
> If so, wouldn’t a more straightforward way to check that your streamoff length
> can fit in a size_type be to just make sure that length >= 0?

No, on Windows, I'm getting 64-bits for std::streamoff and 32-bits for the
size_type.  The static_cast handles the negative cases as well.

Here is how streamoff is defined.  It says that up to GCC 3.3 it was defined as
long, but now it appears to be int64_t:

 /**
...
   *  Note: In versions of GCC up to and including GCC 3.3, streamoff
   *  was typedef long.
  */  

#ifdef _GLIBCXX_HAVE_INT64_T_LONG
  typedef long          streamoff;
#elif defined(_GLIBCXX_HAVE_INT64_T_LONG_LONG)
  typedef long long     streamoff;
#elif defined(_GLIBCXX_HAVE_INT64_T) 
  typedef int64_t       streamoff;
#else
  typedef long long     streamoff;
#endif

[Mark Mentovai]

LGTM