Warns if algorithm is used with `.begin() and .end()` from
different variables.
Details
Diff Detail
- Build Status
Buildable 2194 Build 2194: arc lint + arc unit
Event Timeline
clang-tidy/ClangTidy.cpp | ||
---|---|---|
296 ↗ | (On Diff #81578) | Is this intentional? |
clang-tidy/misc/InvalidRangeCheck.cpp | ||
40 ↗ | (On Diff #81578) | Extra ; in string here or on line 36? |
docs/ReleaseNotes.rst | ||
84 | "invalid" < "use" | |
docs/clang-tidy/checks/misc-invalid-range.rst | ||
12 ↗ | (On Diff #81578) | What about the parallel versions of algorithms? |
clang-tidy/ClangTidy.cpp | ||
---|---|---|
296 ↗ | (On Diff #81578) | actually not, good catch. I was testing alpha checks |
docs/clang-tidy/checks/misc-invalid-range.rst | ||
12 ↗ | (On Diff #81578) | 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 | ||
---|---|---|
41 ↗ | (On Diff #81604) | They're the same object... |
test/clang-tidy/misc-invalid-range.cpp | ||
---|---|---|
41 ↗ | (On Diff #81604) | 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 | 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 | 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 | 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 | 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 | 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 | 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 | Two nits here:
| |
62 | Does this check make sense without the names whitelist? What will is the use case? | |
67 | 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 | Is there a less wasteful way of doing this? E.g. compare pointers to canonical declarations? | |
docs/ReleaseNotes.rst | ||
120 | 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 | I would guess most of the functions that would be called like | |
67 | good point | |
82 | 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 | ||
120 | 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 | 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 | ||
120 | 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 | 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 | I would expect a warning on this line, in that the result of std::move() is unused. |
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.