[clang-tidy] Add check for passing the result of `std::string::c_str` to `strlen`

[acoster]

Summary

Suggests replacing the call with std::string::size.

[Eugene.Zelenko]

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

2

Please make it single string.

36

What about std::basic_string_view? Or any class with c_str/data/length/size?

66

Please fix.

clang-tools-extra/docs/ReleaseNotes.rst

135

Please use double back-ticks for language constructs.

clang-tools-extra/docs/clang-tidy/checks/readability/strlen-string-cstr.rst

7

Please make it same as statement in Release Notes.

23

Please fix.

clang-tools-extra/test/clang-tidy/checkers/readability/strlen-string-cstr.cpp

118

Please fix.

[carlosgalvezp]

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

24

The docs say this is "true" by default, but here you are setting it to false.

clang-tools-extra/docs/clang-tidy/checks/readability/strlen-string-cstr.rst

25

Is there a use case for wanting this option? (As opposed to unconditionally warning about data()) The problem is the same and the same fix applies?

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/readability/strlen-string-cstr.cpp

2

The default case typically doesn't need a suffix.

3

Please give a more descriptive name

36

Tip: you don't need to write duplicate checks, you can simply skip the suffix and it will apply to both RUN lines:

CHECK-MESSAGES: ...
CHECK-FIXES: ...

[acoster]

Generlised the check to cover string_view and "string-like" classes

Removed the option of ignoring data(), and generalised the check to suggest
fixes if the result of data() or c_str() method of any class with a public
length() or size() method is passed to strlen.

[acoster]

clang-tools-extra/docs/clang-tidy/checks/readability/strlen-string-cstr.rst

25

@carlosgalvezp Removed the option.

[Eugene.Zelenko]

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

2

Should be 80 characters long. See other checks as example.

[acoster]

Fix length of comment at the top of StrlenStringCStrCheck.h

[njames93]

I don't see the appear of this check as its a situation that I doubt ever appears in code bases. If there are open source code bases where this is a known problem can you please provide links to them as well as running the run_clang_tidy script over them to verify the changes are good.

[acoster]

In D140968#4029072, @njames93 wrote:

I don't see the appear of this check as its a situation that I doubt ever appears in code bases. If there are open source code bases where this is a known problem can you please provide links to them as well as running the run_clang_tidy script over them to verify the changes are good.

After a look at some projects, it seems like the pattern does arise, but often it's intended (eg a string buffer is set to a large size, a legacy function taking a char* writes to it, and then the string is resized to strlen(foo.c_str())). As such, I'll drop this patch, as it can introduce bugs in these cases.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   RedundantSmartptrGetCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
+  StrlenStringCStrCheck.cpp
   SuspiciousCallArgumentCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
   UppercaseLiteralSuffixCheck.cpp
   UseAnyOfAllOfCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils

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

 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
+#include "StrlenStringCStrCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<RedundantStringInitCheck>(
         "readability-redundant-string-init");
     CheckFactories.registerCheck<SimplifyBooleanExprCheck>(
         "readability-simplify-boolean-expr");
+    CheckFactories.registerCheck<StrlenStringCStrCheck>(
+        "readability-strlen-string-cstr");
     CheckFactories.registerCheck<SuspiciousCallArgumentCheck>(
         "readability-suspicious-call-argument");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
         "readability-uppercase-literal-suffix");
     CheckFactories.registerCheck<UseAnyOfAllOfCheck>(
         "readability-use-anyofallof");

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

+//===--- StrlenStringCStrCheck.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_STRLENSTRINGCSTRCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLENSTRINGCSTRCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds calls to `strlen(std::string::c_str())`.
+class StrlenStringCStrCheck : public ClangTidyCheck {
+public:
+  StrlenStringCStrCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLENSTRINGCSTRCHECK_H

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

+//===- StrlenStringCStrCheck.cpp - Check for strlen(string::c_str()) calls-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements a check for calls to `strlen(std::string::c_str())`.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StrlenStringCStrCheck.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void StrlenStringCStrCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  const auto StrlenFunctions = functionDecl(
+      hasAnyName("::strlen", "::std::strlen", "::strnlen", "::strnlen_s",
+                 "::wcslen", "::std::wcslen", "::wcsnlen_s"));
+  const auto CStrCall = cxxMemberCallExpr(callee(
+      cxxMethodDecl(hasAnyName("c_str", "data"),
+                    ofClass(cxxRecordDecl(has(
+                        cxxMethodDecl(hasAnyName("size", "length"), isPublic())
+                            .bind("size-method")))))));
+  const auto StrlenCall =
+      callExpr(callee(StrlenFunctions), hasArgument(0, CStrCall.bind("arg")))
+          .bind("strlen");
+  Finder->addMatcher(StrlenCall, this);
+}
+
+void StrlenStringCStrCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *StrlenCall = Result.Nodes.getNodeAs<CallExpr>("strlen");
+  const auto *Arg = Result.Nodes.getNodeAs<CXXMemberCallExpr>("arg");
+  const auto *SizeMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("size-method");
+
+  // Remove the `strlen(` (or similar) prefix.
+  CharSourceRange StrlenRange = CharSourceRange::getCharRange(
+      StrlenCall->getBeginLoc(), Arg->getBeginLoc());
+  // Remove the `)` (and additional strnlen arguments, if any) from the strlen
+  // call.
+  CharSourceRange CloseParenRange =
+      CharSourceRange::getCharRange(Arg->getEndLoc(), StrlenCall->getEndLoc());
+
+  const auto *CStrCallee = cast<MemberExpr>(Arg->getCallee());
+  diag(StrlenCall->getBeginLoc(), "redundant call to %0 and %1")
+      << StrlenCall->getDirectCallee() << Arg->getMethodDecl()
+      << FixItHint::CreateRemoval(StrlenRange)
+      << FixItHint::CreateRemoval(CloseParenRange)
+      << FixItHint::CreateReplacement(CStrCallee->getMemberLoc(),
+                                      SizeMethod->getName());
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`misc-use-anonymous-namespace
   Warns when using ``static`` function or variables at global scope, and suggests
   moving them into an anonymous namespace.
 
 - New :doc:`bugprone-standalone-empty <clang-tidy/checks/bugprone/standalone-empty>` check.
 
   Warns when `empty()` is used on a range and the result is ignored. Suggests `clear()`
   if it is an existing member function.
 
+- New :doc:`readability-strlen-string-cstr
+  <clang-tidy/checks/readability/strlen-string-cstr>` check.
+
+  Warns when the return value of ``c_str()`` or ``data()`` is used as an argument for
+  ``strlen``, and suggests using ``size()``or ``length()`` if one of them is a member
+  function.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `readability-redundant-smartptr-get <readability/redundant-smartptr-get.html>`_, "Yes"
    `readability-redundant-string-cstr <readability/redundant-string-cstr.html>`_, "Yes"
    `readability-redundant-string-init <readability/redundant-string-init.html>`_, "Yes"
    `readability-simplify-boolean-expr <readability/simplify-boolean-expr.html>`_, "Yes"
    `readability-simplify-subscript-expr <readability/simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability/static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability/static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability/string-compare.html>`_, "Yes"
+   `readability-strlen-string-cstr <readability/strlen-string-cstr.html>`_, "Yes"
    `readability-suspicious-call-argument <readability/suspicious-call-argument.html>`_,
    `readability-uniqueptr-delete-release <readability/uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability/uppercase-literal-suffix.html>`_, "Yes"
    `readability-use-anyofallof <readability/use-anyofallof.html>`_,
    `zircon-temporary-objects <zircon/temporary-objects.html>`_,
 
 
 .. csv-table:: Aliases..

clang-tools-extra/docs/clang-tidy/checks/readability/strlen-string-cstr.rst

+.. title:: clang-tidy - readability-strlen-string-cstr
+
+readability-strlen-string-cstr
+==============================
+
+Warns when the return value of ``c_str()`` or ``data()`` is used as an argument
+for ``strlen``, and suggests using ``size()``or ``length()`` if one of them is
+a member function.
+
+Example
+-------
+
+.. code-block:: c++
+
+    std::string str1{"foo"}
+
+    // Use str1.size()
+    size_t length = strlen(str1.c_str());
+
+    std::string_view view(str1);
+    // Use view.size()
+    length = strlen(view.data());

clang-tools-extra/test/clang-tidy/checkers/readability/strlen-string-cstr.cpp

+// RUN: %check_clang_tidy %s readability-strlen-string-cstr %t
+int strlen(const char *);
+int strnlen(const char *, int);
+int strnlen_s(const char *, int);
+
+int wcslen(const wchar_t *);
+int wcsnlen_s(const wchar_t *, int);
+
+namespace std {
+template <typename T> class allocator {};
+template <typename T> class char_traits {};
+
+template <typename C, typename T = std::char_traits<C>>
+class basic_string_view {
+public:
+  basic_string_view();
+  basic_string_view(const basic_string_view &other);
+  const C *data() const;
+  int size() const;
+  int length() const;
+};
+
+using string_view = basic_string_view<char>;
+using wstring_view = basic_string_view<wchar_t>;
+
+template <typename C, typename T = std::char_traits<C>,
+          typename A = std::allocator<C>>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  basic_string(const C *, const A &allocator = A());
+  const C *c_str() const;
+  const C *data() const;
+  int size() const;
+  int length() const;
+  operator basic_string_view<C, T>() const;
+};
+
+using wstring = basic_string<wchar_t>;
+using string = basic_string<char>;
+
+int strlen(const char *);
+int wcslen(const wchar_t *);
+} // namespace std
+
+void handlesBasicString() {
+  std::string str1("a", 1);
+  int length = strlen(str1.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant call to 'strlen' {{.*}}
+  // CHECK-FIXES: {{^  }}int length = str1.size();{{$}}
+
+  length = strnlen(str1.data(), 30);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: redundant call to 'strnlen' {{.*}}
+  // CHECK-FIXES: {{^  }}length = str1.size();{{$}}
+
+  const std::string *p1 = &str1;
+  length = std::strlen(p1->c_str()) + 30;
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}length = p1->size() + 30;{{$}}
+
+  std::wstring wstr1;
+  length = wcslen(wstr1.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: redundant call to 'wcslen' {{.*}}
+  // CHECK-FIXES: {{^  }}length = wstr1.size();{{$}}
+
+  const std::wstring &wstr2 = wstr1;
+  length = std::wcslen(wstr2.data());
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: redundant call to {{.*}}
+  // CHECK-FIXES: {{^  }}length = wstr2.size();{{$}}
+}
+
+void handlesStringView() {
+  std::string str1("foo");
+  std::string_view view = str1;
+  int length = strnlen_s(view.data(), 300);
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant call to 'strnlen_s'
+  // {{.*}} CHECK-FIXES: {{^  }}int length = view.size();{{$}}
+
+  std::wstring wstr1;
+  std::wstring_view wview = wstr1;
+  length = wcsnlen_s(wview.data(), 300);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: redundant call to 'wcsnlen_s'
+  // {{.*}} CHECK-FIXES: {{^  }}length = wview.size();{{$}}
+}
+
+class CustomStringClass {
+public:
+  CustomStringClass(int, char);
+  const char *data() const;
+  int length() const;
+
+private:
+  int size() const;
+};
+
+void handlesStringLikeTypes() {
+  CustomStringClass str(123, 'f');
+  int size = strlen(str.data());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: redundant call to
+  // CHECK-FIXES: {{^  }}int size = str.length();{{$}}
+}
+
+class StringWithoutPublicSizeMethod {
+public:
+  StringWithoutPublicSizeMethod(const char *);
+  const char *c_str();
+
+private:
+  int size() const;
+  int length() const;
+};
+
+void ignoresStringTypesWithoutPublicSizeMethod() {
+  StringWithoutPublicSizeMethod str("foo");
+  int length = strlen(str.c_str());
+}