Page MenuHomePhabricator

[clang-tidy] Add obvious-invalid-range
AbandonedPublic

Authored by Prazek on Dec 15 2016, 7:23 AM.

Details

Summary

Warns if algorithm is used with `.begin() and .end()` from
different variables.

Event Timeline

Prazek updated this revision to Diff 81578.Dec 15 2016, 7:23 AM
Prazek retitled this revision from to [clang-tidy] Add misc-invalid-range.
Prazek updated this object.
Prazek added reviewers: alexfh, malcolm.parsons.
Prazek added a subscriber: cfe-commits.
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?

Prazek marked an inline comment as done.Dec 15 2016, 9:07 AM
Prazek added inline comments.
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.
I don't think it makes sense to support parallel algorithms now because they don't have that many users yet.

Prazek updated this revision to Diff 81597.Dec 15 2016, 9:14 AM
  • Moved to obvious
Prazek marked 2 inline comments as done.Dec 15 2016, 9:21 AM
Prazek added inline comments.
docs/ReleaseNotes.rst
84

You mean to change "invalid" to "use"?
It is not about using range, it is about finding broken range.

Prazek updated this revision to Diff 81600.Dec 15 2016, 9:22 AM
  • Small fixes
Prazek retitled this revision from [clang-tidy] Add misc-invalid-range to [clang-tidy] Add obvious-invalid-range.Dec 15 2016, 9:25 AM

What about arguments 3 and 4 of std::list::splice(It, list, It, It)?

clang-tidy/obvious/ObviousTidyModule.cpp
24

misc?

docs/ReleaseNotes.rst
84

The release notes should be in alphabetical order.

Prazek marked 3 inline comments as done.Dec 15 2016, 9:30 AM
Prazek added inline comments.
docs/ReleaseNotes.rst
84

oh sure, fixed that.

Prazek updated this revision to Diff 81603.Dec 15 2016, 9:31 AM
Prazek marked 2 inline comments as done.
Prazek edited edge metadata.
  • Small fixes
Prazek marked an inline comment as done.Dec 15 2016, 9:35 AM

What about arguments 3 and 4 of std::list::splice(It, list, It, It)?

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?

Prazek updated this revision to Diff 81604.Dec 15 2016, 9:38 AM
  • Small fixes

So does FIXIT comment satisfies you for this patch?

Yes.

clang-tidy/obvious/InvalidRangeCheck.cpp
81

blank line at start of function.

docs/clang-tidy/checks/misc-invalid-range.rst
1 ↗(On Diff #81604)

misc.

test/clang-tidy/misc-invalid-range.cpp
1 ↗(On Diff #81604)

misc

test/clang-tidy/misc-invalid-range.cpp
41 ↗(On Diff #81604)

They're the same object...

Prazek marked 2 inline comments as done.Dec 15 2016, 9:53 AM
Prazek updated this revision to Diff 81606.Dec 15 2016, 9:53 AM
  • Small fixes
Prazek added inline comments.Dec 15 2016, 9:55 AM
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.
add_new_check.py can fix it.

Prazek added inline comments.Dec 15 2016, 10:33 AM
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.

Eugene.Zelenko added inline comments.
clang-tidy/misc/InvalidRangeCheck.cpp
78 ↗(On Diff #81578)

Please remove empty line.

docs/ReleaseNotes.rst
88

Probably container will be better word. Same for documentation.

test/clang-tidy/misc-invalid-range.cpp
28 ↗(On Diff #81578)

Please add closing namespace comment and empty line bfore.

Prazek updated this revision to Diff 81616.Dec 15 2016, 10:36 AM
Prazek marked an inline comment as done.
  • removed misc
Prazek updated this revision to Diff 81617.Dec 15 2016, 10:42 AM
Prazek marked an inline comment as done.
  • Small fixes

Also please review https://reviews.llvm.org/D27815 to make it happen

docs/clang-tidy/checks/list.rst
68

add_new_check.py --update-docs

Prazek added inline comments.Dec 15 2016, 11:27 AM
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.

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

staronj added inline comments.Dec 16 2016, 3:21 AM
clang-tidy/misc/InvalidRangeCheck.cpp
19 ↗(On Diff #81578)
38 ↗(On Diff #81578)

Remove std::copy_n (http://en.cppreference.com/w/cpp/algorithm/copy_n) - it doesn't take range.
Remove std::minmax (http://en.cppreference.com/w/cpp/algorithm/minmax) - as above

What about std::iota (http://en.cppreference.com/w/cpp/algorithm/iota)?

sbarzowski requested changes to this revision.Dec 16 2016, 3:28 AM
sbarzowski edited edge metadata.
sbarzowski added inline comments.
clang-tidy/obvious/InvalidRangeCheck.cpp
20–36

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

Same here.

This revision now requires changes to proceed.Dec 16 2016, 3:29 AM
Prazek marked 2 inline comments as done.Dec 16 2016, 4:57 AM
Prazek added inline comments.
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

sbarzowski added inline comments.Dec 16 2016, 8:02 AM
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.

Prazek marked an inline comment as done.Dec 16 2016, 11:09 AM
Prazek added inline comments.
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.
I also want to keep this simple and short, and by making this list less readable (which is not something that some next developer should care about) I make the whole file more readable, because it is shorter.

Prazek added inline comments.Dec 16 2016, 11:24 AM
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::.

aaron.ballman added inline comments.
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
}
}
alexfh added inline comments.Dec 23 2016, 8:18 AM
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:

  1. 80 is hardly "small" and this code is run once per analyzed file, so you're not saving much. Consider just using std::vector.
  2. I think, SmallVector<...> Names(AlgorithmNames.begin(), AlgorithmNames.end()); would be much easier to read.
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" ;)

Prazek added inline comments.Dec 23 2016, 2:38 PM
clang-tidy/obvious/InvalidRangeCheck.cpp
62

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())

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)

Quuxplusone added inline comments.
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.
I'll pick the thread back up in D27815 rather than reopen it here.

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.

Prazek abandoned this revision.Jun 24 2017, 2:59 AM