Correcting compilation warnings.

Correct compilation warning.

 1) Modify src/common/mac/macho_walker.cc to remove a signed  vs unsigned
comparison.

 2) Replace mktemp in test using AutoTmpDir that has been moved from
client/mac/tests to common/tests.

[qsr]

This remove the use of mktemp from unit test because the function is considered
unsafe. This is only to remove warnings from the compiler, because the
replacement is as unsafe (we need a non-existant file to give to the tested
function, so we cannot be safe), but won't trigger compiler warnings. This is
due to a bug in the bug trigger on chromium. If you think this should not be
done (and I'm not certain myself it should), then I'll go back to chromium and
mark the bug as won't fix.

[Ted Mielczarek]

I think we discussed this before. It would be nice to get rid of the warnings,
but changing the code in this way doesn't have any material benefit, so it seems
kind of odd. An alternative fix would be to use mkdtemp() to create a temporary
directory, and then put temporary files in there. I actually wrote a wrapper for
that at one point:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/mac/t...

It'd be easy enough to move that to src/common and use it in all the unit tests.

[Mark Mentovai]

No way.

The warning’s there for a reason. Don’t defeat it without overcoming the
deficiency that the warning’s pointing out.

mkdtemp is one viable option.

[qsr]

Rework on the CL to use AutoTempDir everywhere we were using mktemp.

[Mark Mentovai]

http://breakpad.appspot.com/328001/diff/1004/5009
File src/common/tests/auto_tempdir.h (right):

http://breakpad.appspot.com/328001/diff/1004/5009#newcode53
Line 53: char tempDir[64] = TEMPDIR "/XXXXXXXXXX";
Should be temp_dir.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

http://breakpad.appspot.com/328001/diff/1004/5009#newcode53
Line 53: char tempDir[64] = TEMPDIR "/XXXXXXXXXX";
Ideally this would contain some sort of identification so that left-behind
directories can be associated with Breakpad. breakpad.XXXXXXXX?

http://breakpad.appspot.com/328001/diff/1004/5009#newcode55
Line 55: path = tempDir;
I’d suggest writing

path.assign(temp_dir)

to make it more obvious that path is a std::string and not a char*. If it were a
char*, the assignment would be invalid because temp_dir is about to go out of
scope. Since it’s a std::string, the copy is OK, and writing .assign() makes
this a bit clearer.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode69
Line 69: unlink(entryPath.c_str());
Not having this do a proper recursive removal may be a problem.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode69
Line 69: unlink(entryPath.c_str());
This and the rmdir can be EXPECTs or something too.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode75
Line 75: std::string path;
This should really be path_.

And you should have a DISALLOW_COPY_AND_ASSIGN in a private: section.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode80
Line 80: #endif  // GOOGLE_BREAKPAD_COMMON_TESTS_AUTO_TEMPDIR
Sorry, I read this file first before realizing you were just moving it.

[qsr]

http://breakpad.appspot.com/328001/diff/1004/5009
File src/common/tests/auto_tempdir.h (right):

http://breakpad.appspot.com/328001/diff/1004/5009#newcode53
Line 53: char tempDir[64] = TEMPDIR "/XXXXXXXXXX";
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> Should be temp_dir.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

Done.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode53
Line 53: char tempDir[64] = TEMPDIR "/XXXXXXXXXX";
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> Ideally this would contain some sort of identification so that left-behind
> directories can be associated with Breakpad. breakpad.XXXXXXXX?

Done.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode55
Line 55: path = tempDir;
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> I’d suggest writing
> 
> path.assign(temp_dir)
> 
> to make it more obvious that path is a std::string and not a char*. If it were
a
> char*, the assignment would be invalid because temp_dir is about to go out of
> scope. Since it’s a std::string, the copy is OK, and writing .assign() makes
> this a bit clearer.

Done.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode69
Line 69: unlink(entryPath.c_str());
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> Not having this do a proper recursive removal may be a problem.

Done.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode69
Line 69: unlink(entryPath.c_str());
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> This and the rmdir can be EXPECTs or something too.

Done.

http://breakpad.appspot.com/328001/diff/1004/5009#newcode75
Line 75: std::string path;
On 2011/11/22 17:52:52, Mark Mentovai wrote:
> This should really be path_.

 Ok.

> And you should have a DISALLOW_COPY_AND_ASSIGN in a private: section.

 Mark -> I don't see this macro used anywhere else on breakpad...

[Mark Mentovai]

http://breakpad.appspot.com/328001/diff/8001/9009
File src/common/tests/auto_tempdir.h (right):

http://breakpad.appspot.com/328001/diff/8001/9009#newcode53
Line 53: char temp_dir[64] = TEMPDIR "/breakpad.XXXXXXXXXX";
Why do you need the 64 at all? This would be fine (and safer) as |char
temp_dir[]|.

http://breakpad.appspot.com/328001/diff/8001/9009#newcode79
Line 79: std::string entryPath = path + "/" + entry->d_name;
Remember, not entryPath, but entry_path.

http://breakpad.appspot.com/328001/diff/8001/9009#newcode90
Line 90: };
Instead of DISALLOW_COPY_AND_ASSIGN, just provide a copy constructor declaration
(but not definition), and do the same for operator=. That’s how Breakpad handles
DISALLOW_COPY_AND_ASSIGN. (At least it does in the processor.)

[qsr]

http://breakpad.appspot.com/328001/diff/8001/9009
File src/common/tests/auto_tempdir.h (right):

http://breakpad.appspot.com/328001/diff/8001/9009#newcode53
Line 53: char temp_dir[64] = TEMPDIR "/breakpad.XXXXXXXXXX";
On 2011/11/23 14:07:44, Mark Mentovai wrote:
> Why do you need the 64 at all? This would be fine (and safer) as |char
> temp_dir[]|.

Done.

http://breakpad.appspot.com/328001/diff/8001/9009#newcode79
Line 79: std::string entryPath = path + "/" + entry->d_name;
On 2011/11/23 14:07:44, Mark Mentovai wrote:
> Remember, not entryPath, but entry_path.

Done.

http://breakpad.appspot.com/328001/diff/8001/9009#newcode90
Line 90: };
On 2011/11/23 14:07:44, Mark Mentovai wrote:
> Instead of DISALLOW_COPY_AND_ASSIGN, just provide a copy constructor
declaration
> (but not definition), and do the same for operator=. That’s how Breakpad
handles
> DISALLOW_COPY_AND_ASSIGN. (At least it does in the processor.)

Done.

[Mark Mentovai]

LGTM otherwise

http://breakpad.appspot.com/328001/diff/8002/13009
File src/common/tests/auto_tempdir.h (right):

http://breakpad.appspot.com/328001/diff/8002/13009#newcode67
Line 67: std::string path_;
Normally the data members follow functions.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...