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

Arthur O'Dwyer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 26 12:35:48 PST 2016

Quuxplusone added inline comments.

Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:62
+  auto CallsAlgorithm = hasDeclaration(
+      functionDecl(Names.size() > 0 ? hasAnyName(Names) : anything()));
Prazek wrote:
> 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())
I wonder if there is any hope that STL implementations might one day carry source-level annotations as to what is expected to be a range and what isn't. That is, something like
template<typename _RandomAccessIterator>
inline void
sort(_RandomAccessIterator __first, _RandomAccessIterator __last)
    __attribute__((__expect_range__(__first, __last)))
similar to how we have `__attribute__((__format__))` for printf-style format strings, and `__attribute__((__sentinel__))` for null sentinels, and so on. Then you could eliminate the heuristics concerned with detecting "what formal parameters expect a range" and just work on the heuristics for "what actual arguments are a range". (E.g., v.end() and v.begin() are unlikely to make a valid range in that order. v.begin() and v2.end() are unlikely to make a valid range together. And so on.)

Comment at: docs/ReleaseNotes.rst:120
+  code.
+- New `obvious-invalid-range
+  <http://clang.llvm.org/extra/clang-tidy/checks/obvious-invalid-range.html>`_ check
Prazek wrote:
> 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)
As discussed more thoroughly in https://reviews.llvm.org/D27815 — I continue to think that this is a misuse of the word "obvious". Particularly, the phrase "while looking for an obvious bug" is an oxymoron: if the bug were obvious, you wouldn't need the compiler to help you look for it.
I'll pick the thread back up in D27815 rather than reopen it here.

Comment at: test/clang-tidy/obvious-invalid-range.cpp:35
+  std::copy(v.begin(), v.end(), v2.begin());
I would expect this same check to warn on

    std::copy(v.begin(), v.begin(), v2.begin());
    std::copy(v.end(), v.begin(), v2.begin());

Mind you, I'm not sure *any* of these three warnings will come up in practice enough to be useful to anyone; for all the code it takes to implement them, it might be higher ROI to invest in a general-purpose common-subexpression-detector and/or trying to track "rangeness" through a dataflow analysis.

Comment at: test/clang-tidy/obvious-invalid-range.cpp:45
+  std::move(v.begin(), v.end(), v2.begin());
+  std::move(v.begin());
+  test_copy();
I would expect a warning on this line, in that the result of std::move() is unused.


More information about the cfe-commits mailing list