LGTM
http://breakpad.appspot.com/133001/diff/1/5
File src/common/module.h (right):
http://breakpad.appspot.com/133001/diff/1/5#newcode278
Line 278: FunctionSet functions_; // This module's
functions.
Rather than giving this a heinous amount of horizontal space, why not remove the
heinous space from the preceding line?
If you want these lined up, leave this line as it was (with ;<space><space>//)
and then line up the // on the files_ line above with this one.
http://breakpad.appspot.com/133001/diff/1/2
File src/tools/mac/crash_report/crash_report.xcodeproj/project.pbxproj (left):
http://breakpad.appspot.com/133001/diff/1/2#oldcode392
Line 392: _GLIBCXX_DEBUG_PEDANTIC,
Correct, this stuff doesn’t work with Apple gcc 4.2.
This is part of the price paid for not changing the C++ interfaces when the
compiler was upgraded from 4.0 to 4.2.
I came across some bugs in the patch, not cosmetic issues. http://breakpad.appspot.com/133001/diff/5001/6002 File src/common/dwarf/bytereader.cc (right): ...
I came across some bugs in the patch, not cosmetic issues.
http://breakpad.appspot.com/133001/diff/5001/6002
File src/common/dwarf/bytereader.cc (right):
http://breakpad.appspot.com/133001/diff/5001/6002#newcode126
Line 126: size_t skew = section_base_ & (AddressSize() - 1);
I'm guessing that the reason you changed the types of section_base_ and friends
to size_t in bytereader.h is that you get a warning here. However, as I say in
the comment on that hunk, section_base_ and friends should stay uint64. The way
to avoid a warning here would be to cast to size_t. We know that AddressSize()
fits in a size_t, so we can tell that the value of the expression fits in a
size_t, so the cast is benign.
http://breakpad.appspot.com/133001/diff/5001/6002#newcode128
Line 128: off_t offset = skew + (buffer - buffer_base_);
off_t is the type of the argument to the 'lseek' system call; it is an offset
within a file, and not at all the right thing here. Perhaps you meant ptrdiff_t?
http://breakpad.appspot.com/133001/diff/5001/6003
File src/common/dwarf/bytereader.h (right):
http://breakpad.appspot.com/133001/diff/5001/6003#newcode304
Line 304: size_t section_base_, text_base_, data_base_, function_base_;
This isn't right --- these are target addresses, and could well be 64 bits long.
These should stay uint64.
Issue 133001: Cleanup from review 130001
(Closed)
Created 14 years, 4 months ago by dmaclach
Modified 14 years, 3 months ago
Reviewers: jimb, mochalatte, Mark Mentovai
Base URL: http://google-breakpad.googlecode.com/svn/trunk/
Comments: 5