Turn on more warnings in ios / mac projects.

Turn on more warnings in ios / mac projects.
Make casts explicit.
  This makes casts that loose precision explicit, from here on we will get
warnings.
  The changes in this commit are made without evaluating each cast, asuming the
original
  casts were intentional.
Fixes local variable shadows.

[qsr]

https://breakpad.appspot.com/453002/diff/1/src/client/ios/BreakpadController.mm
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/453002/diff/1/src/client/ios/BreakpadController....
src/client/ios/BreakpadController.mm:227: return uploadIntervalInSeconds_ -
(int)(spanSeconds + 0.5);
no C style cast.

More over we are not at the second, so you can get rid of the + 0.5

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/breakpad_nl...
File src/client/mac/handler/breakpad_nlist_64.cc (right):

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/breakpad_nl...
src/client/mac/handler/breakpad_nlist_64.cc:364: long savpos = (long)(lseek(fd,
0, SEEK_CUR));
No C style cast (here an everywhere on this CL)

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/minidump_ge...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/minidump_ge...
src/client/mac/handler/minidump_generator.cc:432: assert(0 && "Unknown CPU
type!");
The only other case of assert with a string is using assert(!"..."), can you do
the same?

https://breakpad.appspot.com/453002/diff/1/src/common/mac/file_id.cc
File src/common/mac/file_id.cc (right):

https://breakpad.appspot.com/453002/diff/1/src/common/mac/file_id.cc#newcode93
src/common/mac/file_id.cc:93: buffer[buffer_idx++] = (hi >= 10) ? (char)('A' +
hi - 10) : (char)('0' + hi);
Line should be 80 chars or less.

[jakerr]

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/minidump_ge...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/1/src/client/mac/handler/minidump_ge...
src/client/mac/handler/minidump_generator.cc:432: assert(0 && "Unknown CPU
type!");
On 2012/09/11 07:14:13, qsr wrote:
> The only other case of assert with a string is using assert(!"..."), can you
do
> the same?

If I do, we get a warning "Implicit conversion turns string literal into bool:
'const char [18]' to 'bool'"

That's the warning I was attempting to fix.

I guess I missed the other places that use assert(!"...") since they aren't in
the ios project.

[jakerr]

This change is ready for another look.
Made casts c++ style and fixed lines that were too long.

Thank you!

[qsr]

Mark, can you look at the issue in dynamic_image.cc?

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/breakpad_n...
File src/client/mac/handler/breakpad_nlist_64.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/breakpad_n...
src/client/mac/handler/breakpad_nlist_64.cc:364: long savpos =
static_cast<long>(lseek(fd, 0, SEEK_CUR));
Why do you need this cast, changing long to off_t should solve your issue, no?

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
File src/client/mac/handler/dynamic_images.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
src/client/mac/handler/dynamic_images.cc:262: image.vmaddr_ =
static_cast<vm_address_t>(seg->vmaddr);
I'm sorry, but I fear this is not correct (not saying it was before). This
method is parametrized to handle both 32 and 64 bits architecture, but if you
run this on a 32 bits architecture but with a 64 bits archive, vm_address_t will
be 32 bits, and you will lose half of the address.

 Mark: Am I missing something, or the introduction of multi arch support just
broke this in the bad case? Should we change DynamicImage to be either
parametrized (yuck), or always have size of the bigger types?

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/minidump_g...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/minidump_g...
src/client/mac/handler/minidump_generator.cc:1320:
static_cast<uint16_t>((uint32_t)identifier[4] << 8 | identifier[5]);
Instead of casting the all casting the all thing to uint16_t, just change the
uint32_t to uint16_t.

 And you can change all the casts in this part of code to static_cast

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
File src/client/minidump_file_writer_unittest.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
src/client/minidump_file_writer_unittest.cc:89: for (unsigned int i = 0; i <
count; ++i) {
Why don't you change the type of i to unsigned char?

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
src/client/minidump_file_writer_unittest.cc:102: for (unsigned int i = 0; i <
count; ++i) {
Same thing.

[Mark Mentovai]

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
File src/client/mac/handler/dynamic_images.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
src/client/mac/handler/dynamic_images.cc:262: image.vmaddr_ =
static_cast<vm_address_t>(seg->vmaddr);
None of this stuff currently works cross-bitted (or, really, end-to-end on
64-bit x86_64) anyway. vmaddr_ and vmsize_ are mach_vm_address_t and
mach_vm_size_t, so things are going to go to the wider types when stored anyway,
so the casts, if necessary, should use these types instead of truncating.

[Mark Mentovai]

https://breakpad.appspot.com/453002/diff/11/src/client/ios/Breakpad.xcodeproj...
File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right):

https://breakpad.appspot.com/453002/diff/11/src/client/ios/Breakpad.xcodeproj...
src/client/ios/Breakpad.xcodeproj/project.pbxproj:495:
CLANG_WARN_SUSPICIOUS_IMPLICIT_CONVERSION = YES;
Which of these was triggering the warning we’re talking about, and what was the
exact warning?

[jakerr]

Please take another look.

https://breakpad.appspot.com/453002/diff/11/src/client/ios/Breakpad.xcodeproj...
File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right):

https://breakpad.appspot.com/453002/diff/11/src/client/ios/Breakpad.xcodeproj...
src/client/ios/Breakpad.xcodeproj/project.pbxproj:495:
CLANG_WARN_SUSPICIOUS_IMPLICIT_CONVERSION = YES;
On 2012/09/14 13:48:25, Mark Mentovai wrote:
> Which of these was triggering the warning we’re talking about, and what was
the
> exact warning?

CLANG_WARN_SUSPICIOUS_IMPLICIT_CONVERSION gave warnings like:

google3/third_party/breakpad/src/client/ios/BreakpadController.mm:227:35: error:
implicit conversion turns floating-point number into integer: 'double' to 'int'
[-Wconversion]
  return uploadIntervalInSeconds_ - spanSeconds;
  ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~


GCC_WARN_SHADOW like:

google3/third_party/breakpad/src/client/mac/handler/minidump_generator.cc:291:30:
error: declaration shadows a local variable [-Wshadow]
      mach_msg_type_number_t info_count = VM_REGION_SUBMAP_INFO_COUNT_64;


The other's were added for good measure but not sure what the specific warnings
they will provide are.

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/breakpad_n...
File src/client/mac/handler/breakpad_nlist_64.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/breakpad_n...
src/client/mac/handler/breakpad_nlist_64.cc:364: long savpos =
static_cast<long>(lseek(fd, 0, SEEK_CUR));
On 2012/09/14 07:58:47, qsr wrote:
> Why do you need this cast, changing long to off_t should solve your issue, no?

Yes thanks, I was a bit mechanical in adding casts, sorry.

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
File src/client/mac/handler/dynamic_images.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/dynamic_im...
src/client/mac/handler/dynamic_images.cc:262: image.vmaddr_ =
static_cast<vm_address_t>(seg->vmaddr);
On 2012/09/14 13:47:22, Mark Mentovai wrote:
> None of this stuff currently works cross-bitted (or, really, end-to-end on
> 64-bit x86_64) anyway. vmaddr_ and vmsize_ are mach_vm_address_t and
> mach_vm_size_t, so things are going to go to the wider types when stored
anyway,
> so the casts, if necessary, should use these types instead of truncating.

Done.

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/minidump_g...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/mac/handler/minidump_g...
src/client/mac/handler/minidump_generator.cc:1320:
static_cast<uint16_t>((uint32_t)identifier[4] << 8 | identifier[5]);
On 2012/09/14 07:58:47, qsr wrote:
> Instead of casting the all casting the all thing to uint16_t, just change the
> uint32_t to uint16_t.
> 
>  And you can change all the casts in this part of code to static_cast

Done.

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
File src/client/minidump_file_writer_unittest.cc (right):

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
src/client/minidump_file_writer_unittest.cc:89: for (unsigned int i = 0; i <
count; ++i) {
On 2012/09/14 07:58:47, qsr wrote:
> Why don't you change the type of i to unsigned char?

Done.

https://breakpad.appspot.com/453002/diff/11/src/client/minidump_file_writer_u...
src/client/minidump_file_writer_unittest.cc:102: for (unsigned int i = 0; i <
count; ++i) {
On 2012/09/14 07:58:47, qsr wrote:
> Same thing.

Done.

https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/minidum...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/minidum...
src/client/mac/handler/minidump_generator.cc:911: recurse_info =
reinterpret_cast<vm_region_recurse_info_t>(&info);
Using re-interpret cast to eliminate the c-style cast below. This is how other
places in the file treat the cast to vm_region_recurse_info_t so should be what
was intended.

[qsr]

https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
File src/client/mac/handler/dynamic_images.cc (right):

https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
src/client/mac/handler/dynamic_images.cc:1: //
? Why do you remove copyright notice?

[jakerr]

On 2012/09/18 07:55:53, qsr wrote:
>
https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
> File src/client/mac/handler/dynamic_images.cc (right):
> 
>
https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
> src/client/mac/handler/dynamic_images.cc:1: //
> ? Why do you remove copyright notice?

Good question. That was not intentional.

[jakerr]

On 2012/09/18 08:00:01, jakerr wrote:
> On 2012/09/18 07:55:53, qsr wrote:
> >
>
https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
> > File src/client/mac/handler/dynamic_images.cc (right):
> > 
> >
>
https://breakpad.appspot.com/453002/diff/11001/src/client/mac/handler/dynamic...
> > src/client/mac/handler/dynamic_images.cc:1: //
> > ? Why do you remove copyright notice?
> 
> Good question. That was not intentional.

Fixed in patch set 4

[qsr]

https://breakpad.appspot.com/453002/diff/15002/src/client/mac/handler/minidum...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/15002/src/client/mac/handler/minidum...
src/client/mac/handler/minidump_generator.cc:432: assert(0 && "Unknown CPU
type!");
I still don't understand why assert(!"Unknown CPU type!") will trigger a
warning. Are you sure the problem was not with the previous value that was just
plain wrong, as the negation was missing? If you really need to change that,
then there is at least another instance of this in the project, and it must also
be changed.

[jakerr]

On 2012/09/19 11:15:02, qsr wrote:
>
https://breakpad.appspot.com/453002/diff/15002/src/client/mac/handler/minidum...
> File src/client/mac/handler/minidump_generator.cc (right):
> 
>
https://breakpad.appspot.com/453002/diff/15002/src/client/mac/handler/minidum...
> src/client/mac/handler/minidump_generator.cc:432: assert(0 && "Unknown CPU
> type!");
> I still don't understand why assert(!"Unknown CPU type!") will trigger a
> warning. Are you sure the problem was not with the previous value that was
just
> plain wrong, as the negation was missing? If you really need to change that,
> then there is at least another instance of this in the project, and it must
also
> be changed.

Yes, the problem is !"string" is an implicit conversion from string to bool. So
without a cast we get this warning:

mac/handler/minidump_generator.cc:432:14: warning: implicit conversion turns
string literal into bool: 'const char [18]' to 'bool' [-Wstring-conversion]
      assert(!"Unknown CPU type!");
      ~~~~~~^~~~~~~~~~~~~~~~~~~~~

assert(0 && "string") seems to be a common work around for this in other
projects.
I'll update the other instances.

[qsr]

https://breakpad.appspot.com/453002/diff/22001/src/client/mac/handler/minidum...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/22001/src/client/mac/handler/minidum...
src/client/mac/handler/minidump_generator.cc:432: assert(0 &&   "Unknown CPU
type!");
Too much whitespace.

[jakerr]

https://breakpad.appspot.com/453002/diff/22001/src/client/mac/handler/minidum...
File src/client/mac/handler/minidump_generator.cc (right):

https://breakpad.appspot.com/453002/diff/22001/src/client/mac/handler/minidum...
src/client/mac/handler/minidump_generator.cc:432: assert(0 &&   "Unknown CPU
type!");
On 2012/09/20 07:48:10, qsr wrote:
> Too much whitespace.
Oops, sorry. Done.

[qsr]

LGTM

[qsr]

Pushed as revision 1046.

Could you close this review, I cannot do it.