Implement File::WriteAtCurrentPos for Windows

base/files/file_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file.h"
 
 #include <io.h>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ skipping 121 matching lines @@
     return -1;
 
   LARGE_INTEGER offset_li;
   offset_li.QuadPart = offset;
 
   OVERLAPPED overlapped = {0};
   overlapped.Offset = offset_li.LowPart;
   overlapped.OffsetHigh = offset_li.HighPart;
 
   DWORD bytes_read;
-  if (::ReadFile(file_, data, size, &bytes_read, &overlapped) != 0)
+  if (::ReadFile(file_, data, size, &bytes_read, &overlapped))

[cpu_(ooo_6.6-7.5), 2014/03/06 20:43:41]

I am confused about the style unification here, below you seem to want

 if ( WindowsCallThatReuturnsBOOL(...) != FALSE)

But at 142 you dont.

[rvargas (doing something else), 2014/03/06 21:55:34]

I generally don't want to compare against anything (like here), but there are
some places (return statements) where we have to compare against something or
the compiler will warn about casting from int to bool (and fail given that there
is a warning). Between comparing against FALSE and using "!!Foo()" I prefer
comparing against FALSE.

     return bytes_read;
   if (ERROR_HANDLE_EOF == GetLastError())
     return 0;
 
   return -1;
 }

[cpu_(ooo_6.6-7.5), 2014/03/06 20:43:41]

Just a suggestion:

it would look pretty if there was a function here

bool CheckConditions(int size) {
  base::ThreadRestrictions::AssertIOAllowed();
  DCHECK(IsValid());
  return (size < 0);
}

that you could use all over this file, by my count at least 22 times, in two
flavors

if (CheckContions(size))
 return -1;

or just CheckContions(0);

[rvargas (doing something else), 2014/03/06 21:55:34]

That method would look a little suspicious to me as it seems to be about three
lines that are not that connected to each other (especially the size check):
internal object state, usage on the proper thread (both preconditions), and
argument sanity that may or may not apply for a given method.

Also, I'd say that most people care about the implementation of a single
operation, so there is value in having all there is to a function right there.

 
 int File::ReadAtCurrentPos(char* data, int size) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
   DCHECK(!async_);
   if (size < 0)
     return -1;
 
   DWORD bytes_read;
-  if (::ReadFile(file_, data, size, &bytes_read, NULL) != 0)
+  if (::ReadFile(file_, data, size, &bytes_read, NULL))
     return bytes_read;
   if (ERROR_HANDLE_EOF == GetLastError())
     return 0;
 
   return -1;
 }
 
 int File::ReadNoBestEffort(int64 offset, char* data, int size) {
   return Read(offset, data, size);
 }
 
 int File::ReadAtCurrentPosNoBestEffort(char* data, int size) {
   return ReadAtCurrentPos(data, size);
 }
 
 int File::Write(int64 offset, const char* data, int size) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
   DCHECK(!async_);
 
   LARGE_INTEGER offset_li;
   offset_li.QuadPart = offset;
 
   OVERLAPPED overlapped = {0};
   overlapped.Offset = offset_li.LowPart;
   overlapped.OffsetHigh = offset_li.HighPart;
 
   DWORD bytes_written;
-  if (::WriteFile(file_, data, size, &bytes_written, &overlapped) != 0)
+  if (::WriteFile(file_, data, size, &bytes_written, &overlapped))
     return bytes_written;
 
   return -1;
 }
 
 int File::WriteAtCurrentPos(const char* data, int size) {

[Nico, 2014/03/06 23:57:59]

fwiw, i'd have named "write" "writeatoffset" and "writeatcurrentpos" "write",
for consistentcy with fwrite and istream::write etc

-  NOTREACHED();
+  base::ThreadRestrictions::AssertIOAllowed();
+  DCHECK(IsValid());
+  DCHECK(!async_);
+  if (size < 0)
+    return -1;
+
+  DWORD bytes_written;
+  if (::WriteFile(file_, data, size, &bytes_written, NULL))
+    return bytes_written;
+
   return -1;
 }
 
 int File::WriteAtCurrentPosNoBestEffort(const char* data, int size) {
   return WriteAtCurrentPos(data, size);
 }
 
 int64 File::GetLength() {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
   LARGE_INTEGER size;
   if (!::GetFileSizeEx(file_.Get(), &size))
     return -1;
 
   return static_cast<int64>(size.QuadPart);
 }
 
 bool File::SetLength(int64 length) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
 
   // Get the current file pointer.
   LARGE_INTEGER file_pointer;
   LARGE_INTEGER zero;
   zero.QuadPart = 0;
-  if (::SetFilePointerEx(file_, zero, &file_pointer, FILE_CURRENT) == 0)
+  if (!::SetFilePointerEx(file_, zero, &file_pointer, FILE_CURRENT))
     return false;
 
   LARGE_INTEGER length_li;
   length_li.QuadPart = length;
   // If length > file size, SetFilePointerEx() should extend the file
   // with zeroes on all Windows standard file systems (NTFS, FATxx).
   if (!::SetFilePointerEx(file_, length_li, NULL, FILE_BEGIN))
     return false;
 
   // Set the new file length and move the file pointer to its old position.
   // This is consistent with ftruncate()'s behavior, even when the file
   // pointer points to a location beyond the end of the file.
   // TODO(rvargas): Emulating ftruncate details seem suspicious and it is not
   // promised by the interface (nor was promised by PlatformFile). See if this
   // implementation detail can be removed.
-  return ((::SetEndOfFile(file_) != 0) &&
-          (::SetFilePointerEx(file_, file_pointer, NULL, FILE_BEGIN) != 0));
+  return ((::SetEndOfFile(file_) != FALSE) &&
+          (::SetFilePointerEx(file_, file_pointer, NULL, FILE_BEGIN) != FALSE));
 }
 
 bool File::Flush() {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
   return ::FlushFileBuffers(file_) != FALSE;
 }
 
 bool File::SetTimes(Time last_access_time, Time last_modified_time) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
 
   FILETIME last_access_filetime = last_access_time.ToFileTime();
   FILETIME last_modified_filetime = last_modified_time.ToFileTime();
   return (::SetFileTime(file_, NULL, &last_access_filetime,
-                        &last_modified_filetime) != 0);
+                        &last_modified_filetime) != FALSE);
 }
 
 bool File::GetInfo(Info* info) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
 
   BY_HANDLE_FILE_INFORMATION file_info;
-  if (GetFileInformationByHandle(file_, &file_info) == 0)
+  if (!GetFileInformationByHandle(file_, &file_info))
     return false;
 
   LARGE_INTEGER size;
   size.HighPart = file_info.nFileSizeHigh;
   size.LowPart = file_info.nFileSizeLow;
   info->size = size.QuadPart;
   info->is_directory =
       (file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
   info->is_symbolic_link = false;  // Windows doesn't have symbolic links.
   info->last_modified = base::Time::FromFileTime(file_info.ftLastWriteTime);
@@ skipping 53 matching lines @@
                                   last_error);
       return FILE_ERROR_FAILED;
   }
 }
 
 void File::SetPlatformFile(PlatformFile file) {
   file_.Set(file);
 }
 
 }  // namespace base