alexfh requested changes to this revision.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59
@@ -57,1 +58,3 @@
"modernize-use-bool-literals");
+ CheckFactories.registerCheck<UseAlgorithmCheck>(
+ "modernize-use-algorithm");
----------------
Entries should be sorted alphabetically.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:50
@@ +49,3 @@
+
+static std::string getReplacement(const StringRef Function,
+ const StringRef Arg0, const StringRef Arg1,
----------------
I don't think this function pulls its weight. With just two callers and a
trivial implementation, it seems that it's better to inline it.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
@@ +58,5 @@
+ IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+ Options.get("IncludeStyle", "llvm"))) {
+
+ for (const auto &KeyValue :
+ {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {
----------------
I'm not sure this works on MSVC2013. Could someone try this before submitting?
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:86
@@ +85,3 @@
+ // benign.
+ if (getLangOpts().CPlusPlus) {
+ Inserter = llvm::make_unique<utils::IncludeInserter>(
----------------
For consistency, I'd prefer the early return style (`if (!...) return;`).
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
@@ +101,3 @@
+ assert(it != Replacements.end() &&
+ "Replacements list does not match list of registered matcher names");
+ const std::string ReplacedName = it->second;
----------------
Notes are treated completely differently - each note has to be attached to a
warning.
Clang-tidy can only deal with two severity levels of diagnostics: "warning" and
"error", but it's better to let them be controlled by the user via
`-warnings-as-errors=` command-line option or the `WarningsAsErrors`
configuration file option.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:103
@@ +102,3 @@
+ "Replacements list does not match list of registered matcher names");
+ const std::string ReplacedName = it->second;
+
----------------
This should be a StringRef instead. No need to allocate a string.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:131
@@ +130,3 @@
+
+ const StringRef Arg0Text = getText(MatchedExpr->getArg(0), SM, LangOptions);
+ const StringRef Arg1Text = getText(MatchedExpr->getArg(1), SM, LangOptions);
----------------
Should `clang::tooling::fixit::getText` be used instead?
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:20
@@ +19,3 @@
+
+/// Replaces std::memcpy and std::memset with std::copy and std::fill,
+/// respectively.
----------------
Please enclose inline code snippets in backquotes.
Repository:
rL LLVM
https://reviews.llvm.org/D22725
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
