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

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.


================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:62
+  auto CallsAlgorithm = hasDeclaration(
+      functionDecl(Names.size() > 0 ? hasAnyName(Names) : anything()));
+
----------------
alexfh wrote:
>  Does this check make sense without the names whitelist? What will is the use case?
I would guess most of the functions that would be called like
foo(v.begin(), v2.end(), ...) would take range as 2 first arguments. at least I never saw code that this would be valid 
(other case is something like foo(v.begin(), v2.begin(), ...), but I look only for ranges [begin, end())


================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:67
+        hasDeclaration(cxxMethodDecl(hasName(MethodName))),
+        onImplicitObjectArgument(declRefExpr().bind(BindThisName)));
+  };
----------------
alexfh wrote:
> 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.
good point


================
Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:82
+  const auto *SecondArg = Result.Nodes.getNodeAs<DeclRefExpr>("second_arg");
+  if (FirstArg->getNameInfo().getAsString() ==
+      SecondArg->getNameInfo().getAsString())
----------------
alexfh wrote:
> Is there a less wasteful way of doing this? E.g. compare pointers to canonical declarations?
maybe there is. I firstly wanted to make it simple and working and see what people think about these kind of checks.


================
Comment at: docs/ReleaseNotes.rst:120
+  code.
+- New `obvious-invalid-range
+  <http://clang.llvm.org/extra/clang-tidy/checks/obvious-invalid-range.html>`_ check
----------------
alexfh wrote:
> 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" ;)
I runned it on LLVM and clang and as expected it didn't find anything (the code would crash or would be dead)


https://reviews.llvm.org/D27806