This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Checker for inaccurate use of erase method.
ClosedPublic

Authored by xazax.hun on Feb 9 2015, 1:09 AM.

Details

Summary

Checks for inaccurate use of \c erase() method.

Algorithms like \c remove() does not actually remove any element from the
container but returns an iterator to the first redundant element at the end
of the container. These redundant elements must be removed using the
\c erase() method. This check warns when not all of the elements will be
removed due to using an inappropriate overload.

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 19566.Feb 9 2015, 1:09 AM
xazax.hun retitled this revision from to [clang-tidy] Checker for inaccurate use of erase method..
xazax.hun updated this object.
xazax.hun edited the test plan for this revision. (Show Details)
xazax.hun added a reviewer: alexfh.
xazax.hun added a subscriber: Unknown Object (MLST).
xazax.hun added inline comments.Feb 9 2015, 1:12 AM
clang-tidy/misc/InaccurateEraseCheck.cpp
25

It seems to work for, but I have some concerns. Is it guaranteed to work? Is there some kind of guaranteed matching/evaluating order or matcher priority? What is the idiomatic way to optionally match a construct?

alexfh accepted this revision.Feb 9 2015, 7:10 AM
alexfh edited edge metadata.

Looks good!

Thanks for the contribution! Could you run this on LLVM? I don't expect the check to find anything, but it may be useful as a smoke-testing.

A couple of typos in the comment. See below.

clang-tidy/misc/InaccurateEraseCheck.cpp
25

The order of evaluation is right-to-left, and anyOf is short-circuiting, so the construct is guaranteed to work. There was an idea to add an "optionally" matcher for this purpose, but nobody got around to do it yet.

clang-tidy/misc/InaccurateEraseCheck.h
20

nit: s/does not/do not/

21

nit: s/returns/return/

This revision is now accepted and ready to land.Feb 9 2015, 7:10 AM
This revision was automatically updated to reflect the committed changes.

Thank you. I have fixed the issues in the comment and commited in r228679.

alexfh added inline comments.Feb 23 2015, 5:36 AM
clang-tools-extra/trunk/clang-tidy/misc/InaccurateEraseCheck.cpp
27 ↗(On Diff #19648)

Interestingly, the matcher also matches this code:

$ cat test.cc
#include <map>
#include <memory>

void f() {
  std::map<int, std::unique_ptr<int>> m;
  auto iter = m.begin();
  m.erase(iter++);
}

$ clang-tidy test.cc -- -std=c++11
...
test.cc:7:3: warning: this call will remove at most one item even when multiple items should be removed [misc-inaccurate-erase]
  m.erase(iter++);
  ^
...

I suspect that getQualifiedNameAsString() for std::unique_ptr<>::operator++ is matched by the "std::unique" part of the regexp. One possible solution is to enclose the regexp in ^$.

Thank you for finding this issue. The original checker before port used hasName and anyOf. It looks like I should be more cautious when replacing such constructs with regexps. It should be fixed in r230483.

The issue was: the iterator has a template parameter that is substituted for the type that is stored inside the container. The postfix ++ operator has one parameter.