Warns if algorithm is used with `.begin() and .end()` from
different variables.
Details
Diff Detail
- Build Status
Buildable 2176 Build 2176: arc lint + arc unit
Event Timeline
clang-tidy/ClangTidy.cpp | ||
---|---|---|
296 | actually not, good catch. I was testing alpha checks | |
docs/clang-tidy/checks/misc-invalid-range.rst | ||
12 | parallel version won't take iterator as first argument, so it won't gonna match. |
docs/ReleaseNotes.rst | ||
---|---|---|
84 | You mean to change "invalid" to "use"? |
docs/ReleaseNotes.rst | ||
---|---|---|
84 | oh sure, fixed that. |
I am aware of that, but I wanted to make a bunch of simple checks to fill obvious module without taking too much time.
So does FIXIT comment satisfies you for this patch?
test/clang-tidy/misc-invalid-range.cpp | ||
---|---|---|
42 | They're the same object... |
test/clang-tidy/misc-invalid-range.cpp | ||
---|---|---|
42 | Yep, but this type of code stinks, so it is probably a bug |
docs/clang-tidy/checks/list.rst | ||
---|---|---|
68 | misc. |
docs/clang-tidy/checks/list.rst | ||
---|---|---|
68 | How? I added new check with this script, but unfortunatelly rename_check doesn't move check from module to module, so I have to make all the changes by hand. |
docs/clang-tidy/checks/list.rst | ||
---|---|---|
68 | add_new_check.py --update-docs |
docs/clang-tidy/checks/list.rst | ||
---|---|---|
68 | Didn't know about it, cool |
It may be good idea to create LLVM check which will suggest to use STLExtras.h wrappers instead of STL algorithms.
Sure, I actually didn't know about STLExtras, but anyway it would be something LLVM specific, where I want these checks to be non specific and usefull for every day use
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | I would go with one per line. It will be much more diff-friendly this way. And also much easier to add stuff in the middle and maybe keep it sorted. |
40โ45 โ | (On Diff #81617) | Same here. |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | I don't expect this list to change in any way in the future, and it already take more than 20 lines. I don't want to put about 80 lines only to define the names |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | Ok, I don't think it's important enough to argue about it if it was your deliberate decision. But I still think your argument is invalid :-). If anything that calls for putting it in a separate file. |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | By moving it to another file I would expose it to other checks, which is not my intention. |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | ok I think I will remove the list and instead will check if the name starts with std::, because if someone calls std::. |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
20โ36 โ | (On Diff #81617) | You should check if the canonical name starts with ::std::, otherwise you may run into issues with: namespace my { namespace std { // bad code goes here } } |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
38 โ | (On Diff #81617) | No global auto variables, please. In this case auto just isn't buying you anything, but in other cases it may be highly misleading. |
58 โ | (On Diff #81617) | Two nits here:
|
62 โ | (On Diff #81617) | Does this check make sense without the names whitelist? What will is the use case? |
67 โ | (On Diff #81617) | 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. |
82 โ | (On Diff #81617) | Is there a less wasteful way of doing this? E.g. compare pointers to canonical declarations? |
docs/ReleaseNotes.rst | ||
126 | 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" ;) |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
62 โ | (On Diff #81617) | I would guess most of the functions that would be called like |
67 โ | (On Diff #81617) | good point |
82 โ | (On Diff #81617) | maybe there is. I firstly wanted to make it simple and working and see what people think about these kind of checks. |
docs/ReleaseNotes.rst | ||
126 | I runned it on LLVM and clang and as expected it didn't find anything (the code would crash or would be dead) |
clang-tidy/obvious/InvalidRangeCheck.cpp | ||
---|---|---|
62 โ | (On Diff #81617) | 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.) |
docs/ReleaseNotes.rst | ||
126 | 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. | |
test/clang-tidy/obvious-invalid-range.cpp | ||
35 โ | (On Diff #81617) | 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. |
45 โ | (On Diff #81617) | I would expect a warning on this line, in that the result of std::move() is unused. |
Is this intentional?