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

[Ariel Bernal]

  @sarcasm I uploaded the wrong patch but all your comments are still valid. User documentation has to be changed since the user will be able to specify all files in a compilation database. I can add some more in-code documentation too.


================
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 {
----------------
Guillaume Papin wrote:
> 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.
no problem, I think the while-loop looks better here.

================
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.
----------------
Guillaume Papin wrote:
> 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.
sure, I don't think there is performance issue here and it will make the code clear. I'll remove the check and isExcludedListEmpty method.

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

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

================
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
----------------
Guillaume Papin wrote:
> 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);
> 
no problem.


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