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