Add LoggingSeekFile

Add LoggingSeekFile

As the whence values are "helpfully" available in windows.h as well
( http://msdn.microsoft.com/en-us/library/windows/desktop/dd757336(v=vs.85).aspx )
don't bother inventing a new enum. Add implementations for POSIX and Win32.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/384497475a5e0002d2af6a80cd75ee6d3cd69644

[scottmg, 2014-12-19 05:51:21]

Patchset #1 (id:1) has been deleted

[Mark Mentovai, 2014-12-19 14:51:01]

LGTM

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:107: CHECK(whence == SEEK_SET || whence == SEEK_CUR ||
whence == SEEK_END);
I think this one would be OK as a DCHECK. It’s kind of the same thing as the
DCHECK at the top of ReadFile(), like “dummy, you shouldn’t have given that to
me as an argument!” It’s not really the kind of thing that might be passed a
different value in a production build in the field.

As an alternative, you can move this to the switch statement by doing “default:
NOTREACHED();” The style guide wants a “default:” case on switches over
non-enums, per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi....

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:111: case SEEK_SET:
CL description: “both” → “bother”.

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:124: li.LowPart = SetFilePointer(file, li.LowPart,
&li.HighPart, method);
Wow, this is a really weird function! I had to check the documentation. It said
that SetFilePointerEx() would be less cumbersome.

It’s fine with me if you have a reason to not use that. The use of
SetFilePointer() here is correct as-is.

[scottmg, 2014-12-19 17:57:23]

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:107: CHECK(whence == SEEK_SET || whence == SEEK_CUR ||
whence == SEEK_END);
On 2014/12/19 14:51:00, Mark Mentovai wrote:
> I think this one would be OK as a DCHECK. It’s kind of the same thing as the
> DCHECK at the top of ReadFile(), like “dummy, you shouldn’t have given that to
> me as an argument!” It’s not really the kind of thing that might be passed a
> different value in a production build in the field.
> 
> As an alternative, you can move this to the switch statement by doing
“default:
> NOTREACHED();” The style guide wants a “default:” case on switches over
> non-enums, per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi....

Done.

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:111: case SEEK_SET:
On 2014/12/19 14:51:00, Mark Mentovai wrote:
> CL description: “both” → “bother”.

Done.

https://codereview.chromium.org/812593005/diff/40001/util/file/file_io_win.cc...
util/file/file_io_win.cc:124: li.LowPart = SetFilePointer(file, li.LowPart,
&li.HighPart, method);
On 2014/12/19 14:51:00, Mark Mentovai wrote:
> Wow, this is a really weird function! I had to check the documentation. It
said
> that SetFilePointerEx() would be less cumbersome.
> 
> It’s fine with me if you have a reason to not use that. The use of
> SetFilePointer() here is correct as-is.

Ah, for some reason I thought Ex was Vista+. Switched to the Ex version.

[Mark Mentovai, 2014-12-19 18:44:17]

LGTM

[scottmg, 2014-12-19 18:45:28]

Committed patchset #3 (id:60001) manually as
384497475a5e0002d2af6a80cd75ee6d3cd69644 (presubmit successful).