@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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
