alexfh added inline comments.
================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:38
+
+const auto CXX11_AlgorithmNames =
+ CXX_AlgorithmNames + "; "
----------------
No global `auto` variables, please. In this case `auto` just isn't buying you
anything, but in other cases it may be highly misleading.
================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:58
+ // Not so small vector. 80 because there are about that many algorithms.
+ const auto Names =
+ SmallVector<StringRef, 80>(AlgorithmNames.begin(), AlgorithmNames.end());
----------------
Two nits here:
1. 80 is hardly "small" and this code is run once per analyzed file, so you're
not saving much. Consider just using std::vector.
2. I think, `SmallVector<...> Names(AlgorithmNames.begin(),
AlgorithmNames.end());` would be much easier to read.
================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:62
+ auto CallsAlgorithm = hasDeclaration(
+ functionDecl(Names.size() > 0 ? hasAnyName(Names) : anything()));
+
----------------
Does this check make sense without the names whitelist? What will is the use
case?
================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:67
+ hasDeclaration(cxxMethodDecl(hasName(MethodName))),
+ onImplicitObjectArgument(declRefExpr().bind(BindThisName)));
+ };
----------------
Why should this be a `declRefExpr`? This will miss cases with a more complex
expression, e.g. `std::count(x.y().begin(), x.z().end(), ...)`. Considering
`y()` and `z()` are simple getters, this might be a quite common code.
================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:82
+ const auto *SecondArg = Result.Nodes.getNodeAs<DeclRefExpr>("second_arg");
+ if (FirstArg->getNameInfo().getAsString() ==
+ SecondArg->getNameInfo().getAsString())
----------------
Is there a less wasteful way of doing this? E.g. compare pointers to canonical
declarations?
================
Comment at: docs/ReleaseNotes.rst:120
+ code.
+- New `obvious-invalid-range
+ <http://clang.llvm.org/extra/clang-tidy/checks/obvious-invalid-range.html>`_
check
----------------
The idea of the `obvious` module is interesting, but I'm not sure this check
belongs here. At least, I would like to run it on our code and see whether it
finds anything before calling this "obvious" ;)
https://reviews.llvm.org/D27806
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
