[PATCH] D27806: [clang-tidy] Add obvious-invalid-range
Alexander Kornienko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 23 08:18:56 PST 2016
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
More information about the cfe-commits
mailing list