[PATCH] Transform all files in a compilation database if no source files are provided.

[Guillaume Papin]

  The algorithm looks fine, my comments are just to help improve readability a bit.

  In a previous comment you said:
  > Addressed comments, I'll be uploading a new patch with changes to documentation shortly.

  Were you talking about in-source documentation or about the user documentation?


================
Comment at: clang-modernize/tool/ClangModernize.cpp:335
@@ +334,3 @@
+  if (!SourcePaths.empty()) {
+    if (!GlobalOptions.ModifiableFiles.isExcludeListEmpty())
+      // Remove explicitly provided sources that are in the exclude list.
----------------
I would remove this condition. The call to `GlobalOptions.ModifiableFiles.isFileExplicitlyExcluded()` already does the right thing.

Also it bugs me there is no braces around, I known most 1 statement control flow statements with only one element do not require it but when the elements spread on ~9 lines I think it's important to add braces.

If the check is removed the method `isFileExplicitlyExcluded()` can also be removed I guess.

================
Comment at: clang-modernize/tool/ClangModernize.cpp:337-344
@@ +336,10 @@
+      // Remove explicitly provided sources that are in the exclude list.
+      for (std::vector<std::string>::iterator I = SourcePaths.begin();
+           I != SourcePaths.end();)
+        if (GlobalOptions.ModifiableFiles.isFileExplicitlyExcluded(*I)) {
+          llvm::errs() << "Warning \"" << *I << "\" will not be transformed "
+                       << "because it's in the excluded list.\n";
+          I = SourcePaths.erase(I);
+        } else
+          ++I;
+  } else {
----------------
I like a while-loop better here and there won't be any issues "leaking" the `I` variable since it's the only content of the compound statement.

  std::vector<std::string>::iterator I = SourcePaths.begin();
  while (I != SourcePaths.end()) {
    if (GlobalOptions.ModifiableFiles.isFileExplicitlyExcluded(*I)) {
      llvm::errs() << "Warning \"" << *I << "\" will not be transformed "
                   << "because it's in the excluded list.\n";
      I = SourcePaths.erase(I);
    } else
      ++I;
  }

But this is totally a personal opinion so feel free to ignore.

================
Comment at: clang-modernize/tool/ClangModernize.cpp:345-346
@@ +344,4 @@
+          ++I;
+  } else {
+    if (!GlobalOptions.ModifiableFiles.isIncludeListEmpty()) {
+      // Use source paths from the compilation database.
----------------
I think we can simply use `else if (!GlobalOptions.ModifiableFiles.isIncludeListEmpty())` thus avoiding an unnecessary inner block.

================
Comment at: clang-modernize/tool/ClangModernize.cpp:349-354
@@ +348,8 @@
+      std::vector<std::string> Files = Compilations->getAllFiles();
+      for (std::vector<std::string>::const_iterator I = Files.begin(),
+                                                    E = Files.end();
+           I != E; ++I)
+        // We only transform files that are explicitly included.
+        if (GlobalOptions.ModifiableFiles.isFileIncluded(*I))
+          SourcePaths.addValue(*I);
+    } else
----------------
I would rewrite this slightly differently using `std::copy_if()`, it requires to create a small predicate but I believe it makes the intent clearer.

  std::copy_if(Files.begin(), Files.end(), std::back_inserter(SourcePaths), isFileIncludedPredicate);


================
Comment at: clang-modernize/tool/ClangModernize.cpp:360-362
@@ +359,5 @@
+
+  for (unsigned i = 0; i < SourcePaths.size(); ++i) {
+    llvm::errs() << SourcePaths[i] << "\n";
+  }
+
----------------
Is this some debugging code?


http://llvm-reviews.chandlerc.com/D1517