[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:34
     return;
 
   const auto ValidContainer = qualType(hasUnqualifiedDesugaredType(
----------------
```
auto ContainerLenghtFuncNames = anyOf(hasName("size"), hasName("length"));
```
pick better naem


================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:40
                       isConst(), parameterCountIs(0), isPublic(),
-                      hasName("size"),
+                      anyOf(hasName("size"), hasName("length")),
                       returns(qualType(isInteger(), unless(booleanType()))))
----------------
s/`anyOf(hasName("size"), hasName("length")),`/`ContainerLenghtFuncNames,`/


================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:40-65
+                      anyOf(hasName("size"), hasName("length")),
                       returns(qualType(isInteger(), unless(booleanType()))))
                       .bind("size")),
               has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
                                 hasName("empty"), returns(booleanType()))
                       .bind("empty")))
               .bind("container")))))));
----------------
lebedev.ri wrote:
> s/`anyOf(hasName("size"), hasName("length")),`/`ContainerLenghtFuncNames,`/
We want `anyOf()` in both cases, with the same list of suspects.
If it is not a `anyOf()` in the second case, then naturally it won't work (can't have more than one name)
If it is not a `anyOf()` in the first case, then it will e.g. work if all of them are present.
So we just want to ensure that we do the same thing in both cases.



================
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:65
                                       hasType(references(ValidContainer))))),
-                        callee(cxxMethodDecl(hasName("size"))), WrongUse,
+                        callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))), WrongUse,
                         unless(hasAncestor(cxxMethodDecl(
----------------
lebedev.ri wrote:
> This line looks too long, clang-format might be too intrusive, so at least
> ```
>                         callee(cxxMethodDecl(anyOf(hasName("size"), 
>                                                    hasName("length")))),
>                         WrongUse,
> 
> ```
s/`callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))), WrongUse,`/`callee(cxxMethodDecl(ContainerLenghtFuncNames)), WrongUse,`/


================
Comment at: test/clang-tidy/readability-container-size-empty.cpp:19-20
   basic_string<T> operator+(const basic_string<T>& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;
----------------
Quolyk wrote:
> lebedev.ri wrote:
> > Does it still work if only one of these exists?
> It works indeed, do you suggest adding test case for this?
Hmm, actually, i think there is an easier solution, see other inline comment.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644