alexfh requested changes to this revision.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:37
@@ +36,3 @@
+static bool areTypesCompatible(QualType Left, QualType Right) {
+ return getStrippedType(Left) == getStrippedType(Right);
+}
----------------
Please inline this function as well. It's only used once and the implementation
+ the comment that is already in the call site are more than enough to explain
the intent.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:41
@@ +40,3 @@
+static bool noPointerArithmetic(QualType T) {
+ return T->isVoidPointerType() || T->isFunctionPointerType();
+}
----------------
I'd probably inline this function as well. Its name is not particularly clear
anyway (could be better with 'doesNotSupportPointerArithmetic` or similar, but
I think, inlining it is even better).
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:48
@@ +47,3 @@
+ Options.get("IncludeStyle", "llvm"))) {
+ Replacements["memcpy"] = "std::copy";
+ Replacements["memset"] = "std::fill";
----------------
Actually, I don't think we need a map of exactly two entries, since the order
of arguments still has to be hardcoded. Let's just do a couple of `if`s or a
`StringSwitch` in the `check()` method.
================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:141
@@ +140,3 @@
+ Result.SourceManager->getFileID(Loc), "algorithm",
+ /*IsAngled=*/true)) {
+ Diag << *IncludeFixit;
----------------
No braces around single-statement `if` bodies, please.
================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:51
@@ +50,3 @@
+
+ std::copy(foo ? bar : baz, foo ? baz : baz + size, dest);
+
----------------
I think, we need to fix this right away instead of documenting this bug. Two
things here:
1. problems related to the operator precedence can be fixed by adding
parentheses, when needed (see how a similar issue was solved in
`SimplifyBooleanExprCheck`; here we can make a `needsParensBeforePlus` function
similar to `needsParensAfterUnaryNegation`).
2. the check should avoid replacements that duplicate an expression with side
effects that can result from increment/decrement operators, assignments and
some function calls. Function calls is the most difficult part here, since not
all function calls have side effects (or are expensive). One approach here
could be to add a `const auto &` variable to keep the value of the argument we
need to repeat, if it's anything more complex than just a DeclRefExpr.
Repository:
rL LLVM
https://reviews.llvm.org/D22725
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
