[PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D24349#538102, @aaron.ballman wrote:

> In https://reviews.llvm.org/D24349#538098, @alexfh wrote:
>
> > Thank you for the patch!
> >
> > In https://reviews.llvm.org/D24349#537500, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote:
> > >
> > > > Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.
> > >
> > >
> > > I think this should actually have two options: one is a list of default-checked containers (default is the list of 
> > >  STL containers we previously had), and the other is a Boolean option for guessing at what might be a 
> > >  container based on function signature (default is true). This gives people a way to silence the diagnostic 
> > >  while still being useful.
> >
> >
> > While I understand your concerns, I would suggest to wait for actual bug reports before introducing more options. The duck typing approach worked really well for readability-redundant-smartptr-get and I expect it to work equally well here. The constraints checked here are pretty strict and should make the check safe.
>
>
> That means there is no way to silence this check on false positives aside from disabling it entirely, which is not a particularly good user experience. However, I do agree that if we constrain the heuristic appropriately, the number of false positives should be low.


Just checked: readability-container-size-empty with the patch finds a few thousand new warnings related to about 400 unique container names in our code. A representative sample shows no false positives and we've found just a single false negative (that triggers the check without the patch) - a container's `size()` method was not `const`. Which makes me pretty sure that:

1. The new algorithm is much more consistent and useful than the whitelist approach (gathering and maintaining a whitelist of a few hundred names is also not fun at all).
2. Given the size of our code and the number of third-party libraries, possibility of false positives is extremely low.
3. If there are reports of false positives, we could go with a blacklist approach instead, but I don't think this will ever be needed.


================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+  const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom(
+      namedDecl(has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
+                                  hasName("size"), returns(isInteger()),
----------------
I don't think we can come up with a proper solution until we see a real world example. If we want to find one faster, we can allow all character types and wait for reports of false positives ;)

================
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18
@@ +17,3 @@
+
+Check issues warning if a container has `size()` and `empty()` methods matching
+following signatures:
----------------
`The check`

================
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18
@@ +17,3 @@
+
+Check issues warning if a container has `size()` and `empty()` methods matching
+following signatures:
----------------
alexfh wrote:
> `The check`
Double backquotes should be used.

================
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:21
@@ +20,3 @@
+
+code-block::c++
+
----------------
nit: space before `c++`.

================
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:23
@@ +22,3 @@
+
+  size_type size() const;
+  bool empty() const;
----------------
Explain the constraints `size_type` should satisfy.


https://reviews.llvm.org/D24349