Add a new check, readability-simplify-subscript-expr, that diagnoses array subscript expressions that can be simplified.

Currently, diagnoses code that calls container.data()[some_index] when the container exposes a suitable operator[]() method that can be used directly.

Patch by Shuai Wang.

llvm-svn: 332519

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

@@ -25,6 +25,7 @@ add_clang_library(clangTidyReadabilityModule
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp

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

@@ -32,6 +32,7 @@
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
+#include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
@@ -72,6 +73,8 @@ public:
         "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>(

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

@@ -0,0 +1,76 @@
+//===--- 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

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

@@ -0,0 +1,38 @@
+//===--- 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-tools-extra/docs/ReleaseNotes.rst

@@ -151,6 +151,11 @@ Improvements to clang-tidy
   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.
 

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

@@ -227,6 +227,7 @@ Clang-Tidy Checks
    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

clang-tools-extra/docs/clang-tidy/checks/readability-simplify-subscript-expr.rst

@@ -0,0 +1,23 @@
+.. 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`

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

@@ -0,0 +1,108 @@
+// 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);
+}