teemperor added a comment.
See my inline comments about technical changes, but otherwise this looks ready
to land. Please update and I'll give green light ASAP. Thanks!
================
Comment at: lib/Analysis/CloneDetection.cpp:375
+ const Decl *D = S.getContainingDecl();
+ const SourceManager &SM = D->getASTContext().getSourceManager();
+ std::string Filename = std::string(SM.getFilename(D->getLocation()));
----------------
You can skip the `const Decl *D = S.getContainingDecl();` and just do `const
SourceManager &SM = S.getASTContext().getSourceManager();`
================
Comment at: lib/Analysis/CloneDetection.cpp:378
+ // Get Basename
+ const size_t LastSlash = Filename.find_last_of("\\/");
+ if (LastSlash != std::string::npos)
----------------
Let's get the basename with the LLVM path class instead:
http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys_1_1path.html#a799b002e67dcf41330fa8d6fa11823dc
E.g. `moc_test\.cpp` is a valid filename on Linux and then this code isn't
doing the right thing.
================
Comment at: lib/Analysis/CloneDetection.cpp:383
+ if (LastDot != std::string::npos)
+ Filename.erase(LastDot);
+ llvm::Regex R(StringRef("^(" + IgnoredFilesPattern.str() + "$)"));
----------------
Hmm, we can't filter by file extension this way? Can we remove this extension
stripping and just make people type the file extension in the filter, this
feels more intuitive.
================
Comment at: lib/Analysis/CloneDetection.cpp:384
+ Filename.erase(LastDot);
+ llvm::Regex R(StringRef("^(" + IgnoredFilesPattern.str() + "$)"));
+ if (R.match(StringRef(Filename)))
----------------
Artem suggested we could move this Regex out of the loop, I think we should
even make it a member of the AutoGeneratedCloneConstraint so that we only
parse/generate the regex state machine once per invocation. Currently we
reparse this Regex a few thousand times and performance is quite important in
this Checker.
Repository:
rL LLVM
https://reviews.llvm.org/D31320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
