[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 ↗

(On Diff #142701)

Please separate with empty line.

32 ↗

(On Diff #142701)

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 ↗

(On Diff #142701)

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

96

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

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

6 ↗

(On Diff #142701)

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

22 ↗

(On Diff #143204)

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

6 ↗

(On Diff #143204)

Please use as much of 80 characters as possible.

8 ↗

(On Diff #143204)

operator[] should be enclosed in `, not .

14 ↗

(On Diff #143204)

Please insert empty line above.

22 ↗

(On Diff #143204)

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

[shuaiwang]

Addressed more comments.

clang-tidy/readability/RedundantDataCallCheck.cpp

22 ↗

(On Diff #143204)

Done, but why?

[Eugene.Zelenko]

clang-tidy/readability/RedundantDataCallCheck.cpp

22 ↗

(On Diff #143204)

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

45 ↗

(On Diff #143207)

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

57 ↗

(On Diff #143207)

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

7 ↗

(On Diff #143207)

doing array subscript -> doing an array subscript

[Eugene.Zelenko]

clang-tidy/readability/RedundantDataCallCheck.cpp

45 ↗

(On Diff #143207)

readability-redundant-string-cstr do both.

[aaron.ballman]

clang-tidy/readability/RedundantDataCallCheck.cpp

31 ↗

(On Diff #143207)

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

45 ↗

(On Diff #143207)

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

45 ↗

(On Diff #143207)

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

45 ↗

(On Diff #143207)

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

45 ↗

(On Diff #143207)

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

45 ↗

(On Diff #143207)

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

54–55

Can remove the braces.

63

Remove braces.

clang-tidy/readability/SimplifySubscriptExprCheck.h

23

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
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #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 "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
+    CheckFactories.registerCheck<SimplifySubscriptExprCheck>(
+        "readability-simplify-subscript-expr");
     CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>(
         "readability-static-accessed-through-instance");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<StringCompareCheck>(
         "readability-string-compare");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");

clang-tidy/readability/SimplifySubscriptExprCheck.h

+//===--- SimplifySubscriptExprCheck.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_SIMPLIFYSUBSCRIPTEXPRCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFYSUBSCRIPTEXPRCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Simplifies subscript expressions.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-subscript-expr.html
+class SimplifySubscriptExprCheck : public ClangTidyCheck {
+public:
+  SimplifySubscriptExprCheck(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_SIMPLIFYSUBSCRIPTEXPRCHECK_H

clang-tidy/readability/SimplifySubscriptExprCheck.cpp

+//===--- SimplifySubscriptExprCheck.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 "SimplifySubscriptExprCheck.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 {
+
+static const char kDefaultTypes[] =
+    "::std::basic_string;::std::basic_string_view;::std::vector;::std::array";
+
+SimplifySubscriptExprCheck::SimplifySubscriptExprCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), Types(utils::options::parseStringList(
+                                         Options.get("Types", kDefaultTypes))) {
+}
+
+void SimplifySubscriptExprCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  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 SimplifySubscriptExprCheck::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(),
+           "accessing an element of the container does not require a call to "
+           "'data()'; did you mean to use 'operator[]'?");
+  if (Member->isArrow())
+    DiagBuilder << FixItHint::CreateInsertion(Member->getLocStart(), "(*")
+                << FixItHint::CreateInsertion(Member->getOperatorLoc(), ")");
+  DiagBuilder << FixItHint::CreateRemoval(
+      {Member->getOperatorLoc(), Call->getLocEnd()});
+}
+
+void SimplifySubscriptExprCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Types", utils::options::serializeStringList(Types));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - 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
   <clang-tidy/checks/android-comparison-in-temp-failure-retry>` check.
 
   Diagnoses comparisons that appear to be incorrectly placed in the argument to
   the ``TEMP_FAILURE_RETRY`` macro.
- 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:`readability-simplify-subscript-expr
+  <clang-tidy/checks/readability-simplify-subscript-expr>` check.
+
+  Simplifies subscript expressions like ``s.data()[i]`` into ``s[i]``.
+
 - New :doc:`zircon-temporary-objects
   <clang-tidy/checks/zircon-temporary-objects>` check.
 
   Warns on construction of specific temporary objects in the Zircon kernel.
 
 - Added the missing bitwise assignment operations to
   :doc:`hicpp-signed-bitwise <clang-tidy/checks/hicpp-signed-bitwise>`.
 

docs/clang-tidy/checks/list.rst

.. toctree::
    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-multiple-inheritance
    fuchsia-overloaded-operator
    fuchsia-restrict-system-includes
    fuchsia-statically-constructed-objects
    fuchsia-trailing-return
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
.. toctree::
    readability-redundant-control-flow
    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-simplify-subscript-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-simplify-subscript-expr.rst

+.. title:: clang-tidy - readability-simplify-subscript-expr
+
+readability-simplify-subscript-expr
+===================================
+
+This check simplifies subscript expressions. Currently this covers calling
+``.data()`` and immediately doing an array subscript operation to obtain a
+single element, in which case simply calling ``operator[]`` suffice.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string s = ...;
+  char c = s.data()[i];  // char c = s[i];
+
+Options
+-------
+
+.. option:: Types
+
+   The list of type(s) that triggers this check. Default is
+   `::std::basic_string;::std::basic_string_view;::std::vector;::std::array`

test/clang-tidy/readability-simplify-subscript-expr.cpp

+// RUN: %check_clang_tidy %s readability-simplify-subscript-expr %t \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: readability-simplify-subscript-expr.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: accessing an element of the container does not require a call to 'data()'; did you mean to use 'operator[]'? [readability-simplify-subscript-expr]
+  // CHECK-FIXES: int x = v[i];
+
+  std::string s;
+  char c1 = s.data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: accessing an element
+  // CHECK-FIXES: char c1 = s[i];
+
+  std::string_view sv;
+  char c2 = sv.data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: accessing an element
+  // CHECK-FIXES: char c2 = sv[i];
+
+  std::string* ps = &s;
+  char c3 = ps->data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: accessing an element
+  // CHECK-FIXES: char c3 = (*ps)[i];
+
+  char c4 = (*ps).data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: accessing an element
+  // CHECK-FIXES: char c4 = (*ps)[i];
+
+  DO(char c5 = s.data()[i]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: accessing an element
+  // CHECK-FIXES: DO(char c5 = s[i]);
+
+  char c6 = ACCESS(s).data()[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: accessing an element
+  // CHECK-FIXES: char c6 = ACCESS(s)[i];
+
+  char c7 = ACCESS(s.data())[i];
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: accessing an element
+  // CHECK-FIXES: char c7 = ACCESS(s)[i];
+
+  char c8 = ACCESS(s.data()[i]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: accessing an element
+  // CHECK-FIXES: char c8 = ACCESS(s[i]);
+
+  char c9 = GET(s, i);
+
+  char c10 = Foo<std::string>{}.bar(i);
+}