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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
