[clang-tidy] Add a new check, readability-simplify-subscript-expr, that simplifies subscript expressions.

[Eugene.Zelenko]

Please also take a look on 26817 for another idea for .data().

clang-tidy/readability/RedundantDataCallCheck.cpp

20

Please separate with empty line.

32

Isn't this list redundant for using namespace clang::ast_matchers?

docs/ReleaseNotes.rst

63

I would suggest Finds redundant .data() calls. Same in documentation.

Please also move to new checks list in alphabetical order.

docs/clang-tidy/checks/readability-redundant-data-call.rst

14

Please look on other check documentation for example of option descriptions.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

63

Please enclose .data() in ``. Same in documentation.

[aaron.ballman]

Have you run this over any large code bases to see whether the check triggers in practice?

docs/clang-tidy/checks/list.rst

95

This change looks unrelated to the patch and should be handled separately.

docs/clang-tidy/checks/readability-redundant-data-call.rst

6

What does it mean for a data() call to be redundant? I think the documentation needs to explain that a bit more.

[shuaiwang]

Addressed review comments.

[Eugene.Zelenko]

clang-tidy/readability/RedundantDataCallCheck.cpp

23

Actually you should use static, not anonymous namespace for variables and functions.

docs/ReleaseNotes.rst

143

r is after p :-)

146

Please remove This check and enclose .data() in `, not . Same for documentation.

docs/clang-tidy/checks/readability-redundant-data-call.rst

7

Please use as much of 80 characters as possible.

9

operator[] should be enclosed in `, not .

15

Please insert empty line above.

23

I think will be good idea to specify default value as complete string.

[shuaiwang]

Addressed more comments.

clang-tidy/readability/RedundantDataCallCheck.cpp

23

Done, but why?

[Eugene.Zelenko]

clang-tidy/readability/RedundantDataCallCheck.cpp

23

See LLVM coding standards.

[aaron.ballman]

Have you run this over any large code bases to see whether the check triggers in practice?

I'm still curious about this, btw.

clang-tidy/readability/RedundantDataCallCheck.cpp

46

Should this check apply equally to std::string::c_str() as well as std::string::data()?

58

I think the word "redundant" isn't going to be immediately obvious to a user who would write code like that. Perhaps "accessing an element of the container does not require a call to 'data()'; did you mean to use 'operator[]()' instead?"

docs/clang-tidy/checks/readability-redundant-data-call.rst

8

doing array subscript -> doing an array subscript

[Eugene.Zelenko]

clang-tidy/readability/RedundantDataCallCheck.cpp

46

readability-redundant-string-cstr do both.

[aaron.ballman]

clang-tidy/readability/RedundantDataCallCheck.cpp

32

No need to register any of these matchers unless in C++ mode.

46

Yup! But that makes me wonder if the name "redundant-data-call" is an issue. Perhaps the check name should focus more on the array subscript in the presence of an operator[]()?

[shuaiwang]

clang-tidy/readability/RedundantDataCallCheck.cpp

46

How about "readability-circumlocutionary-subscript"?
"readability-circumlocutionary-element-access"?
"circumlocutionary" -> "verbose"?

[shuaiwang]

In D45702#1085224, @aaron.ballman wrote:

Have you run this over any large code bases to see whether the check triggers in practice?

I'm still curious about this, btw.

Yes it triggers in Google's code base.

[aaron.ballman]

In D45702#1085890, @shuaiwang wrote:

In D45702#1085224, @aaron.ballman wrote:

Have you run this over any large code bases to see whether the check triggers in practice?

I'm still curious about this, btw.

Yes it triggers in Google's code base.

Were there any false positives that you saw?

[shuaiwang]

In D45702#1086250, @aaron.ballman wrote:

In D45702#1085890, @shuaiwang wrote:

In D45702#1085224, @aaron.ballman wrote:

Have you run this over any large code bases to see whether the check triggers in practice?

I'm still curious about this, btw.

Yes it triggers in Google's code base.

Were there any false positives that you saw?

From randomly checking several triggerings no I didn't find any false positives. I feel the check should be pretty safe in terms of false positives because we only trigger on configured types.

[aaron.ballman]

In D45702#1086802, @shuaiwang wrote:

In D45702#1086250, @aaron.ballman wrote:

In D45702#1085890, @shuaiwang wrote:

In D45702#1085224, @aaron.ballman wrote:

Have you run this over any large code bases to see whether the check triggers in practice?

I'm still curious about this, btw.

Yes it triggers in Google's code base.

Were there any false positives that you saw?

From randomly checking several triggerings no I didn't find any false positives. I feel the check should be pretty safe in terms of false positives because we only trigger on configured types.

Good to hear, thank you for the information.

clang-tidy/readability/RedundantDataCallCheck.cpp

46

hah, I think circumlocutionary might be a bit too much. ;-) I think readability-simplify-array-subscript-expr might be reasonable, however. Right now, the simplification is just for foo.data()[0] but it seems plausible that there are other array subscript simplifications that could be added in the future, like a[1 + 1] being converted to a[2] or x ? a[200] : a[400] going to a[x ? 200 : 400] (etc).

[shuaiwang]

clang-tidy/readability/RedundantDataCallCheck.cpp

46

Just readability-simplify-subscript-expr?
Since after simplification the subscript operation is done by calling overloaded operator[] on an object instead of built-in subscript operator on an array.

Let me know if this name looks good to you and I'll do the actual renaming (together with addressing other comments) after your confirmation.

[aaron.ballman]

clang-tidy/readability/RedundantDataCallCheck.cpp

46

I think readability-simplify-subscript-expr is a reasonable name.

[shuaiwang]

Rename to readability-simplify-subscript-expr and addressed other comments.

[aaron.ballman]

LGTM with a few minor nits to be fixed.

clang-tidy/readability/SimplifySubscriptExprCheck.cpp

53–54 ↗

(On Diff #146514)

Can remove the braces.

62 ↗

(On Diff #146514)

Remove braces.

clang-tidy/readability/SimplifySubscriptExprCheck.h

22 ↗

(On Diff #146514)

This URL looks stale.

[shuaiwang]

Addressed review comments.

[aaron.ballman]

In D45702#1097294, @shuaiwang wrote:

Addressed review comments.

This patch was approved; do you need someone to commit this for you?

[shuaiwang]

In D45702#1101136, @aaron.ballman wrote:

In D45702#1097294, @shuaiwang wrote:

Addressed review comments.

This patch was approved; do you need someone to commit this for you?

Yes please :)

[aaron.ballman]

Committed in r332519, thank you for the patch!

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   InconsistentDeclarationParameterNameCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
+  RedundantDataCallCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
+#include "RedundantDataCallCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<RedundantDataCallCheck>(
+        "readability-redundant-data-call");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
     CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>(
         "readability-static-accessed-through-instance");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");

clang-tidy/readability/RedundantDataCallCheck.h

+//===--- RedundantDataCallCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTDATACALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTDATACALLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant .data() calls.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-data-call.html
+class RedundantDataCallCheck : public ClangTidyCheck {
+public:
+  RedundantDataCallCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap& Opts) override;
+
+private:
+  const std::vector<std::string> Types;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTDATACALLCHECK_H

clang-tidy/readability/RedundantDataCallCheck.cpp

+//===--- RedundantDataCallCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantDataCallCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+namespace {
+const char kDefaultTypes[] =
+    "::std::basic_string;::std::basic_string_view;::std::vector;::std::array";
+} // namespace
+
+RedundantDataCallCheck::RedundantDataCallCheck(StringRef Name,
+                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), Types(utils::options::parseStringList(
+                                         Options.get("Types", kDefaultTypes))) {
+}
+
+void RedundantDataCallCheck::registerMatchers(MatchFinder *Finder) {
+  using ast_matchers::allOf;
+  using ast_matchers::anyOf;
+  using ast_matchers::arraySubscriptExpr;
+  using ast_matchers::callee;
+  using ast_matchers::cxxMemberCallExpr;
+  using ast_matchers::cxxRecordDecl;
+  using ast_matchers::has;
+  using ast_matchers::hasAnyName;
+  using ast_matchers::hasBase;
+  using ast_matchers::hasDeclaration;
+  using ast_matchers::hasDescendant;
+  using ast_matchers::hasName;
+  using ast_matchers::hasType;
+  using ast_matchers::hasUnqualifiedDesugaredType;
+  using ast_matchers::ignoringParenImpCasts;
+  using ast_matchers::isArrow;
+  using ast_matchers::memberExpr;
+  using ast_matchers::namedDecl;
+  using ast_matchers::on;
+  using ast_matchers::pointee;
+  using ast_matchers::pointerType;
+  using ast_matchers::qualType;
+  using ast_matchers::recordType;
+  using ast_matchers::substTemplateTypeParmType;
+  using ast_matchers::unless;
+
+  const auto TypesMatcher = hasUnqualifiedDesugaredType(
+      recordType(hasDeclaration(cxxRecordDecl(hasAnyName(
+          llvm::SmallVector<StringRef, 8>(Types.begin(), Types.end()))))));
+
+  Finder->addMatcher(
+      arraySubscriptExpr(hasBase(ignoringParenImpCasts(
+          cxxMemberCallExpr(
+              has(memberExpr().bind("member")),
+              on(hasType(qualType(
+                  unless(anyOf(substTemplateTypeParmType(),
+                               hasDescendant(substTemplateTypeParmType()))),
+                  anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)))))),
+              callee(namedDecl(hasName("data"))))
+              .bind("call")))),
+      this);
+}
+
+void RedundantDataCallCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  if (Result.Context->getSourceManager().isMacroBodyExpansion(
+          Call->getExprLoc())) {
+    return;
+  }
+  const auto *Member = Result.Nodes.getNodeAs<MemberExpr>("member");
+  auto DiagBuilder = diag(Member->getMemberLoc(), "redundant call to .data()");
+  if (Member->isArrow()) {
+    DiagBuilder << FixItHint::CreateInsertion(Member->getLocStart(), "(*")
+                << FixItHint::CreateInsertion(Member->getOperatorLoc(), ")");
+  }
+  DiagBuilder << FixItHint::CreateRemoval(
+      {Member->getOperatorLoc(), Call->getLocEnd()});
+}
+
+void RedundantDataCallCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Types", utils::options::serializeStringList(Types));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`readability-redundant-data-call
+  <clang-tidy/checks/readability-redundant-data-call>` check
+
+  This check suggests removing redundant `.data()` calls.
+
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
 
 - New module ``portability``.
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
 - New :doc:`android-comparison-in-temp-failure-retry
- New :doc:`hicpp-multiway-paths-covered
   Checks on ``switch`` and ``if`` - ``else if`` constructs that do not cover all possible code paths.
 
 - New :doc:`modernize-use-uncaught-exceptions
   <clang-tidy/checks/modernize-use-uncaught-exceptions>` check
 
   Finds and replaces deprecated uses of ``std::uncaught_exception`` to
   ``std::uncaught_exceptions``.
 
 - New :doc:`portability-simd-intrinsics
   <clang-tidy/checks/portability-simd-intrinsics>` check
 
   Warns or suggests alternatives if SIMD intrinsics are used which can be replaced by
   ``std::experimental::simd`` operations.
 
 - New :doc:`zircon-temporary-objects
   <clang-tidy/checks/zircon-temporary-objects>` check
 
   Warns on construction of specific temporary objects in the Zircon kernel.
 
 - Adding the missing bitwise assignment operations to 

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
-   fuchsia-header-anon-namespaces (redirects to google-build-namespaces) <fuchsia-header-anon-namespaces>
    fuchsia-default-arguments
+   fuchsia-header-anon-namespaces (redirects to google-build-namespaces) <fuchsia-header-anon-namespaces>
    fuchsia-multiple-inheritance
    fuchsia-overloaded-operator
    fuchsia-statically-constructed-objects
    fuchsia-trailing-return
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
.. toctree::
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name
    readability-misleading-indentation
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
+   readability-redundant-data-call
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
    zircon-temporary-objects

docs/clang-tidy/checks/readability-redundant-data-call.rst

+.. title:: clang-tidy - readability-redundant-data-call
+
+readability-redundant-data-call
+===============================
+
+This check suggests removing redundant `.data()` calls.
+
+Examples:
+
+.. code-block:: c++
+  std::string s = ...;
+  char c = s.data()[i];  // char c = s[i];
+
+The type(s) that triggers this check can be configured with `Types` option.

test/clang-tidy/readability-redundant-data-call.cpp

+// RUN: %check_clang_tidy %s readability-redundant-data-call %t \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-data-call.Types, \
+// RUN:   value: '::std::basic_string;::std::basic_string_view;MyVector'}]}" --
+
+namespace std {
+
+template <class T>
+class basic_string {
+ public:
+   using size_type = unsigned;
+   using value_type = T;
+   using reference = value_type&;
+   using const_reference = const value_type&;
+
+   reference operator[](size_type);
+   const_reference operator[](size_type) const;
+   T* data();
+   const T* data() const;
+};
+
+using string = basic_string<char>;
+
+template <class T>
+class basic_string_view {
+ public:
+  using size_type = unsigned;
+  using const_reference = const T&;
+  using const_pointer = const T*;
+
+  constexpr const_reference operator[](size_type) const;
+  constexpr const_pointer data() const noexcept;
+};
+
+using string_view = basic_string_view<char>;
+
+}
+
+template <class T>
+class MyVector {
+ public:
+  using size_type = unsigned;
+  using const_reference = const T&;
+  using const_pointer = const T*;
+
+  const_reference operator[](size_type) const;
+  const T* data() const noexcept;
+};
+
+#define DO(x) do { x; } while (false)
+#define ACCESS(x) (x)
+#define GET(x, i) (x).data()[i]
+
+template <class T>
+class Foo {
+ public:
+  char bar(int i) {
+    return x.data()[i];
+  }
+ private:
+  T x;
+};
+
+void f(int i) {
+  MyVector<int> v;
+  int x = v.data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to .data() [readability-redundant-data-call]
+  // CHECK-FIXES: int x = v[i];
+
+  std::string s;
+  char c1 = s.data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant call to .data()
+  // CHECK-FIXES: char c1 = s[i];
+
+  std::string_view sv;
+  char c2 = sv.data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call to .data()
+  // CHECK-FIXES: char c2 = sv[i];
+
+  std::string* ps = &s;
+  char c3 = ps->data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant call to .data()
+  // CHECK-FIXES: char c3 = (*ps)[i];
+
+  char c4 = (*ps).data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant call to .data()
+  // CHECK-FIXES: char c4 = (*ps)[i];
+
+  DO(char c5 = s.data()[i]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant call to .data()
+  // CHECK-FIXES: DO(char c5 = s[i]);
+
+  char c6 = ACCESS(s).data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: redundant call to .data()
+  // CHECK-FIXES: char c6 = ACCESS(s)[i];
+
+  char c7 = ACCESS(s.data())[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+  // CHECK-FIXES: char c7 = ACCESS(s)[i];
+
+  char c8 = ACCESS(s.data()[i]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+  // CHECK-FIXES: char c8 = ACCESS(s[i]);
+
+  char c9 = GET(s, i);
+
+  char c10 = Foo<std::string>{}.bar(i);
+}