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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

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.

> I would document what function signatures we use as our heuristic, but note that the function signatures we care about may change (for instance, we may decide that const-qualification is unimportant, or that we want to require begin()/end() functions, etc). I think `<standard integer type> size() const` and `bool empty() const` are a reasonable heuristic for a first pass.


Yep, documentation should be updated accordingly.


================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+  const auto validContainer = namedDecl(
+      has(functionDecl(
+          isPublic(), hasName("size"), returns(isInteger()),
----------------
aaron.ballman wrote:
> omtcyfz wrote:
> > Thank you!
> > 
> > Blacklisted these types.
> > 
> > Actually, I believe if someone has `bool size()` in their codebase there's still a problem...
> > Blacklisted these types.
> 
> I'm still not certain this is correct. For instance, if someone wants to do `uint8_t size() const;`, we shouldn't punish them because `uint8_t` happens to be `unsigned char` for their platform. But you are correct that we don't want this to work with char32_t, for instance.
> 
> I think the best approach is to make a local matcher for the type predicate. We want isInteger() unless it's char (that's not explicitly qualified with signed or unsigned), bool, wchar_t, char16_t, char32_t, or an enumeration type.
> 
> > Actually, I believe if someone has bool size() in their codebase there's still a problem...
> 
> Agreed, though not as part of this check. ;-)
Same here, let's wait for a real user asking to support his real container class that defines `uint8_t size() const;`.

================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:145
@@ -144,3 +144,3 @@
   }
   diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
                                   "for emptiness instead of 'size'")
----------------
Since we're expanding the set of supported classes, it makes sense to include the name of the class to the diagnostic message.


Repository:
  rL LLVM

https://reviews.llvm.org/D24349