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

[PiotrZSL]

Summary

Extends readability-container-size-empty to check std::string length() similar to size().

Fixes: #37603

Co-authored-by: Dmitry Venikov <[e-mail]>

[lebedev.ri]

Either this is a NFC change with just tests, or the actual fix is missing.

[Quolyk]

For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).

1. Do I have to extend ValidContainer, so it recognises ::std::string with length() method as valid container too or we can assume ::std::string as valid container by default?
2. If ::std::string is valid container, I just add one more expression to match. Is it correct way?

[Quolyk]

In D56644#1355434, @lebedev.ri wrote:

Either this is a NFC change with just tests, or the actual fix is missing.

Right, it's WIP for now.

[lebedev.ri]

In D56644#1355436, @Quolyk wrote:

For now, I just added tests. I have several questions, as I'm beginner (ContainerSizeEmptyCheck.cpp).

1. Do I have to extend ValidContainer, so it recognises ::std::string with length() method as valid container too or we can assume ::std::string as valid container by default?
2. If ::std::string is valid container, I just add one more expression to match. Is it correct way?

https://github.com/llvm-mirror/clang-tools-extra/blob/27011d1db0ee4955bab595ab51ebe264e160ed89/clang-tidy/readability/ContainerSizeEmptyCheck.cpp#L40
i think you want to change ValidContainer to not only check for size, but for size or lenght.
I.e. replace hasName("lenght"), with anyOf(hasName("size"), hasName("lenght")),.
And then adjust the diags to dynamically get the actual method name.

[Quolyk]

Update tests. Handle length.

[lebedev.ri]

Seems to look good.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

65 ↗

(On Diff #181467)

This line looks too long, clang-format might be too intrusive, so at least

callee(cxxMethodDecl(anyOf(hasName("size"), 
                           hasName("length")))),
WrongUse,

test/clang-tidy/readability-container-size-empty.cpp

19–20 ↗

(On Diff #181467)

Does it still work if only one of these exists?

[Quolyk]

test/clang-tidy/readability-container-size-empty.cpp

19–20 ↗

(On Diff #181467)

It works indeed, do you suggest adding test case for this?

[lebedev.ri]

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

34 ↗

(On Diff #181467)

auto ContainerLenghtFuncNames = anyOf(hasName("size"), hasName("length"));

pick better naem

40 ↗

(On Diff #181467)

s/anyOf(hasName("size"), hasName("length")),/ContainerLenghtFuncNames,/

40–65 ↗

(On Diff #181467)

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.

65 ↗

(On Diff #181467)

s/callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))), WrongUse,/callee(cxxMethodDecl(ContainerLenghtFuncNames)), WrongUse,/

test/clang-tidy/readability-container-size-empty.cpp

19–20 ↗

(On Diff #181467)

Hmm, actually, i think there is an easier solution, see other inline comment.

[Eugene.Zelenko]

It'll be worth to mention change in Release Notes (in changes list, in alphabetical order).

[JonasToth]

Could you please run it over LLVM or another significant codebase and check if there are miss-transformations (run with fix and then compile (+ tests optimally))?
You can use clang-tidy/tool/run-clang-tidy.py for a parallel runner.

[MyDeveloperDay]

test/clang-tidy/readability-container-size-empty.cpp

135 ↗

(On Diff #181467)

could you add a test that checks if StringRef.str().length() >0 becomes !StringRef.str.empty()

e.g.

LLVM::StringRef ArgRef;
....
return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()";

[alexfh]

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

65 ↗

(On Diff #181467)

Please use hasAnyName(x, y) instead of anyOf(hasName(x), hasName(y)). It's shorter and executes faster (the more arguments it has, the faster it is compared to the corresponding anyOf(hasName(...), ...) construct).

207 ↗

(On Diff #181467)

Did you try without ->getName()? IIUC, it should work as well.

[JonasToth]

any news here? do you plan to finish this patch @Quolyk ?
Do you mind if someone comandeers this patch?

[Eugene.Zelenko]

It'll be great to finalize patch and close issue #37603.

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[PiotrZSL]

rebase, add support for dependent context, update documentation

[PiotrZSL]

test/clang-tidy/readability-container-size-empty.cpp

135 ↗

(On Diff #181467)

No point, should work anyway.

[PiotrZSL]

Rebase + Ping

[carlosgalvezp]

LGTM!

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h

 //===--- ContainerSizeEmptyCheck.h - clang-tidy -----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTYCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTYCHECK_H
 
 #include "../ClangTidyCheck.h"
 #include <vector>
 
 namespace clang::tidy::readability {
 
-/// Checks whether a call to the `size()` method can be replaced with a call to
-/// `empty()`.
+/// Checks whether a call to the `size()`/`length()` method can be replaced with
+/// a call to `empty()`.
 ///
 /// The emptiness of a container should be checked using the `empty()` method
 /// instead of the `size()` method. It is not guaranteed that `size()` is a
 /// constant-time function, and it is generally more efficient and also shows
 /// clearer intent to use `empty()`. Furthermore some containers may implement
 /// the `empty()` method but not implement the `size()` method. Using `empty()`
 /// whenever possible makes it easier to switch to another container in the
 /// future.

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

 
 void ContainerSizeEmptyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ExcludedComparisonTypes",
                 utils::options::serializeStringList(ExcludedComparisonTypes));
 }
 
 void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
   const auto ValidContainerRecord = cxxRecordDecl(isSameOrDerivedFrom(
-      namedDecl(
-          has(cxxMethodDecl(
-                  isConst(), parameterCountIs(0), isPublic(), hasName("size"),
-                  returns(qualType(isIntegralType(), unless(booleanType()))))
+      namedDecl(has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
+                                  hasAnyName("size", "length"),
+                                  returns(qualType(isIntegralType(),
+                                                   unless(booleanType()))))
                         .bind("size")),
                 has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
                                   hasName("empty"), returns(booleanType()))
                         .bind("empty")))
           .bind("container")));
 
   const auto ValidContainerNonTemplateType =
       qualType(hasUnqualifiedDesugaredType(
           recordType(hasDeclaration(ValidContainerRecord))));
   const auto ValidContainerTemplateType =
       qualType(hasUnqualifiedDesugaredType(templateSpecializationType(
           hasDeclaration(classTemplateDecl(has(ValidContainerRecord))))));
 
   const auto ValidContainer = qualType(
       anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType));
 
   const auto WrongUse =
       anyOf(hasParent(binaryOperator(
                           isComparisonOperator(),
                           hasEitherOperand(anyOf(integerLiteral(equals(1)),
                                                  integerLiteral(equals(0)))))
                           .bind("SizeBinaryOp")),
             usedInBooleanContext());
 
   Finder->addMatcher(
-      cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer),
-                                      hasType(pointsTo(ValidContainer)),
-                                      hasType(references(ValidContainer))))
-                               .bind("MemberCallObject")),
-                        callee(cxxMethodDecl(hasName("size"))), WrongUse,
-                        unless(hasAncestor(cxxMethodDecl(
-                            ofClass(equalsBoundNode("container"))))))
+      cxxMemberCallExpr(
+          on(expr(anyOf(hasType(ValidContainer),
+          callee(
+              cxxMethodDecl(hasAnyName("size", "length")).bind("SizeMethod")),
+          WrongUse,
+          unless(hasAncestor(
+              cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
 
   Finder->addMatcher(
       callExpr(has(cxxDependentScopeMemberExpr(
                        hasObjectExpression(
                            expr(anyOf(hasType(ValidContainer),
                                       hasType(pointsTo(ValidContainer)),
                                       hasType(references(ValidContainer))))
                                .bind("MemberCallObject")),
-                   hasMemberName("size"))),
+                       anyOf(hasMemberName("size"), hasMemberName("length")))
+                       .bind("DependentExpr")),
                WrongUse,
                unless(hasAncestor(
                    cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
 
   // Comparison to empty string or empty constructor.
   const auto WrongComparend =
if (BinCmp) {
     else
       Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
                                           "!" + ReplacementText);
   }
 
   auto WarnLoc = MemberCall ? MemberCall->getBeginLoc() : SourceLocation{};
 
   if (WarnLoc.isValid()) {
-    diag(WarnLoc, "the 'empty' method should be used to check "
-                  "for emptiness instead of 'size'")
-        << Hint;
+    auto Diag = diag(WarnLoc, "the 'empty' method should be used to check "
+                              "for emptiness instead of %0");
+    if (const auto *SizeMethod =
+            Result.Nodes.getNodeAs<NamedDecl>("SizeMethod"))
+      Diag << SizeMethod;
+    else if (const auto *DependentExpr =
+                 Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>(
+                     "DependentExpr"))
+      Diag << DependentExpr->getMember();
+    else
+      Diag << "unknown method";
+    Diag << Hint;
   } else {
     WarnLoc = BinCmpTempl
                   ? BinCmpTempl->getBeginLoc()
                   : (BinCmp ? BinCmp->getBeginLoc()
                             : (BinCmpRewritten ? BinCmpRewritten->getBeginLoc()
                                                : SourceLocation{}));
     diag(WarnLoc, "the 'empty' method should be used to check "
                   "for emptiness instead of comparing to an empty object")

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`performance-noexcept-swap
   match with the swap function signature, eliminating false-positives.
 
 - Improved :doc:`readability-braces-around-statements
   <clang-tidy/checks/readability/braces-around-statements>` check to
   ignore false-positive for ``if constexpr`` in lambda expression.
 
 - Improved :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` check to
-  detect comparison between string and empty string literals.
+  detect comparison between string and empty string literals and support
+  ``length()`` method as an alternative to ``size()``.
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check to emit proper
   warnings when a type forward declaration precedes its definition and
   added support for ``Leading_upper_snake_case`` naming convention.
 
 - Improved :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take

clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst

 .. title:: clang-tidy - readability-container-size-empty
 
 readability-container-size-empty
 ================================
 
 
-Checks whether a call to the ``size()`` method can be replaced with a call to
-``empty()``.
+Checks whether a call to the ``size()``/``length()`` method can be replaced
+with a call to ``empty()``.
 
 The emptiness of a container should be checked using the ``empty()`` method
-instead of the ``size()`` method. It is not guaranteed that ``size()`` is a
-constant-time function, and it is generally more efficient and also shows
-clearer intent to use ``empty()``. Furthermore some containers may implement
-the ``empty()`` method but not implement the ``size()`` method. Using
-``empty()`` whenever possible makes it easier to switch to another container in
-the future.
+instead of the ``size()``/``length()`` method. It is not guaranteed that
+``size()``/``length()`` is a constant-time function, and it is generally more
+efficient and also shows clearer intent to use ``empty()``. Furthermore some
+containers may implement the ``empty()`` method but not implement the ``size()``
+or ``length()`` method. Using ``empty()`` whenever possible makes it easier to
+switch to another container in the future.
 
-The check issues warning if a container has ``size()`` and ``empty()`` methods
-matching following signatures:
+The check issues warning if a container has ``empty()`` and ``size()`` or
+``length()`` methods matching following signatures:
 
 .. code-block:: c++
 
   size_type size() const;
+  size_type length() const;
   bool empty() const;
 
 `size_type` can be any kind of integer type.
 
 .. option:: ExcludedComparisonTypes
 
     A semicolon-separated list of class names for which the check will ignore
     comparisons of objects with default-constructed objects of the same type.
     If a class is listed here, the check will not suggest using ``empty()``
     instead of such comparisons for objects of that class.
     Default value is: `::std::array`.

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string

struct basic_string {
 
   ~basic_string();
 
   const C *c_str() const;
   const C *data() const;
 
   bool empty() const;
   size_type size() const;
+  size_type length() const;
 
   _Type& append(const C *s);
   _Type& append(const C *s, size_type n);
   _Type& assign(const C *s);
   _Type& assign(const C *s, size_type n);
 
   int compare(const _Type&) const;
   int compare(const C* s) const;

clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

 }
 
 bool returnsBool() {
   std::set<int> intSet;
   std::string str;
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if (intSet.empty()){{$}}
   // CHECK-MESSAGES: :21:8: note: method 'set'::empty() defined here
   if (intSet == std::set<int>())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
   // CHECK-FIXES: {{^  }}if (intSet.empty()){{$}}
   // CHECK-MESSAGES: :21:8: note: method 'set'::empty() defined here
   if (s_func() == "")
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
     ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
     ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if (str + str2 == "")
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
     ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == L"")
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   std::vector<int> vect;
   if (vect.size() == 0)
     ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector<int>())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect.size() != 0)
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
template <typename T> void f() {
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   CHECKSIZE(templated_container);
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
+  std::basic_string<T> s;
+  if (s.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
+  if (s.length())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
 }
 
 void g() {
   f<int>();
   f<double>();
   f<char *>();
 }
 

 };
 
 typedef std::array<int, 10U> Array;
 
 bool testArraySize(const Array& value) {
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :735:8: note: method 'array'::empty() defined here
+// CHECK-MESSAGES: :[[@LINE-25]]:8: note: method 'array'::empty() defined here
 }
 
 bool testArrayCompareToEmpty(const Array& value) {
   return value == std::array<int, 10U>();
 }
 
 bool testDummyType(const DummyType& value) {
   return value == DummyType();
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :745:8: note: method 'DummyType'::empty() defined here
+// CHECK-MESSAGES: :[[@LINE-26]]:8: note: method 'DummyType'::empty() defined here
 }
 
 bool testIgnoredDummyType(const IgnoredDummyType& value) {
   return value == IgnoredDummyType();
 }
 
 bool testStringLiterals(const std::string& s)
 {