[PATCH] D27806: [clang-tidy] Add obvious-invalid-range

[Alexander Kornienko via Phabricator via cfe-commits]

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