win: Implementation of CrashReportDatabase for Windows (for C++ Windows readability review)

util/test/scoped_temp_dir_posix.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
@@ skipping 27 matching lines @@
   return base::FilePath(dir_template);
 }
 
 // static
 void ScopedTempDir::RecursivelyDeleteTemporaryDirectory(
     const base::FilePath& path) {
   DIR* dir = opendir(path.value().c_str());
   ASSERT_TRUE(dir) << ErrnoMessage("opendir") << " " << path.value();
 
   dirent* entry;
   while ((entry = readdir(dir))) {

[Peter Kasting, 2015/02/13 00:32:40]

Nit: Consider avoiding using a with-side-effect statement as a loop test.  For
example:

  for (dirent* entry = readdir(dir); entry; entry = readdir(dir)) {

This is only one line, avoids testing the result of side effect statements, and
also places |entry| into a narrower scope (see previous comment).

[scottmg, 2015/02/13 06:34:05]

I defer to Robert/Mark if they prefer that in this _posix file. (I could go
either way -- I don't like the duplication of readdir() in the for-loop version.

[Peter Kasting, 2015/02/13 22:30:33]

Part of the history here is that a few months back I enabled an MSVC warning
that bans "while (a = b)", and I fixed by using a for() loop in those cases. 
It's possible to avoid the warning by adding an extra set of parens as you've
done here but there is at least some level of precedent for doing it the other
way.

[Mark Mentovai, 2015/02/13 22:36:15]

scottmg wrote:
I missed this appeal the first time I read through the comments. I don’t like
the duplication either. This is a well-understood warning and the extra
parentheses to tell the compiler that you mean what you say are also
well-understood.

[Peter Kasting, 2015/02/13 22:38:26]

OK.  I defer to you guys.  I think the primary benefit of the for () construct
is moving the scope of |entry| to be limited to the loop body, but if that
doesn't outweigh this, that's OK.

[Mark Mentovai, 2015/02/13 22:42:27]

Peter Kasting wrote:
I do prefer the for for that reason when it can be done without duplication, but
the duplication is worse than the wider-than-necessary scoping in my opinion.

     if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) {
       continue;
     }
 
     base::FilePath entry_path = path.Append(entry->d_name);
     if (entry->d_type == DT_DIR) {
       RecursivelyDeleteTemporaryDirectory(entry_path);
     } else {
       EXPECT_EQ(0, unlink(entry_path.value().c_str()))
           << ErrnoMessage("unlink") << " " << entry_path.value();
     }
   }
 
   EXPECT_EQ(0, closedir(dir))
       << ErrnoMessage("closedir") << " " << path.value();
   EXPECT_EQ(0, rmdir(path.value().c_str()))
       << ErrnoMessage("rmdir") << " " << path.value();
 }
 
 }  // namespace test
 }  // namespace crashpad
+// EOF comment added for readability review.