[clang-tidy] Remove gsl::at suggestion from cppcoreguidelines-pro-bounds-constant-array-index

[carlosgalvezp]

Summary

Currently the fix hint is hardcoded to gsl::at(). This poses
a problem for people who, for a number of reasons, don't want
or cannot use the GSL library (introducing a new third-party
dependency into a project is not a minor task).

In these situations, the fix hint does more harm than good
as it creates confusion as to what the fix should be. People
can even misinterpret the fix "gsl::at" as e.g. "std::array::at",
which can lead to even more trouble (e.g. when having guidelines
that disallow exceptions).

Furthermore, this is not a requirement from the C++ Core Guidelines.
simply that array indexing needs to be safe. Each project should
be able to decide upon a strategy for safe indexing.

The fix-it is kept for people who want to use the GSL library.

[carlosgalvezp]

Fix alphabetical order in release notes.

[carlosgalvezp]

Update commit message.

[salman-javed-nz]

LGTM besides a couple of nits. I think the diagnostic message should just say what the problem is, not what the fix is - that's what the FixitHint is for.

clang-tools-extra/docs/ReleaseNotes.rst

163

164

To match "C++ Core Guidelines" in the previous bullet point.

[carlosgalvezp]

Fixed review comments

[carlosgalvezp]

Fixed comments

clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

if (const auto *ArraySubscriptE = dyn_cast<ArraySubscriptExpr>(Matched))
       BaseRange = ArraySubscriptE->getBase()->getSourceRange();
     else
       BaseRange =
           dyn_cast<CXXOperatorCallExpr>(Matched)->getArg(0)->getSourceRange();
     SourceRange IndexRange = IndexExpr->getSourceRange();
 
     auto Diag = diag(Matched->getExprLoc(),
                      "do not use array subscript when the index is "
-                     "not an integer constant expression; use gsl::at() "
-                     "instead");
+                     "not an integer constant expression");
     if (!GslHeader.empty()) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
            << FixItHint::CreateReplacement(
                   SourceRange(BaseRange.getEnd().getLocWithOffset(1),
                               IndexRange.getBegin().getLocWithOffset(-1)),
                   ", ")
            << FixItHint::CreateReplacement(Matched->getEndLoc(), ")")
            << Inserter.createMainFileIncludeInsertion(GslHeader);

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-flp37-c
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Removed suggestion ``use gsl::at`` from warning message in the
+  ``cppcoreguidelines-pro-bounds-constant-array-index`` check, since that is not
+  a requirement from the C++ Core Guidelines. This allows people to choose
+  their own safe indexing strategy. The fix-it is kept for those who want to
+  use the GSL library.
+
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
 
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst

 .. title:: clang-tidy - cppcoreguidelines-pro-bounds-constant-array-index
 
 cppcoreguidelines-pro-bounds-constant-array-index
 =================================================
 
 This check flags all array subscript expressions on static arrays and
 ``std::arrays`` that either do not have a constant integer expression index or
 are out of bounds (for ``std::array``). For out-of-bounds checking of static
 arrays, see the `-Warray-bounds` Clang diagnostic.
 
 This rule is part of the "Bounds safety" profile of the C++ Core Guidelines, see
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-bounds-arrayindex.
 
+Optionally, this check can generate fixes using ``gsl::at`` for indexing.
+
 Options
 -------
 
 .. option:: GslHeader
 
    The check can generate fixes after this option has been set to the name of
    the include file that contains ``gsl::at()``, e.g. `"gsl/gsl.h"`.
 
 .. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp

 }
 
 constexpr int const_index(int base) {
   return base + 3;
 }
 
 void f(std::array<int, 10> a, int pos) {
   a [ pos / 2 /*comment*/] = 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
   // CHECK-FIXES: gsl::at(a,  pos / 2 /*comment*/) = 1;
   int j = a[pos - 1];
-  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression
   // CHECK-FIXES: int j = gsl::at(a, pos - 1);
 
   a.at(pos-1) = 2; // OK, at() instead of []
   gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
 
   a[-1] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
   a[10] = 4;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements) [cppcoreguidelines-pro-bounds-constant-array-index]
 
   a[const_index(7)] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
 
   a[0] = 3; // OK, constant index and inside bounds
   a[1] = 3; // OK, constant index and inside bounds
   a[9] = 3; // OK, constant index and inside bounds
   a[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 void g() {
   int a[10];
   for (int i = 0; i < 10; ++i) {
     a[i] = i;
-    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression
     // CHECK-FIXES: gsl::at(a, i) = i;
     gsl::at(a, i) = i; // OK, gsl::at() instead of []
   }
 
   a[-1] = 3; // flagged by clang-diagnostic-array-bounds
   a[10] = 4; // flagged by clang-diagnostic-array-bounds
   a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
 

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-bounds-constant-array-index.cpp

 }
 
 constexpr int const_index(int base) {
   return base + 3;
 }
 
 void f(std::array<int, 10> a, int pos) {
   a [ pos / 2 /*comment*/] = 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
   int j = a[pos - 1];
-  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression
 
   a.at(pos-1) = 2; // OK, at() instead of []
   gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
 
   a[-1] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
   a[10] = 4;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements) [cppcoreguidelines-pro-bounds-constant-array-index]
 
   a[const_index(7)] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
 
   a[0] = 3; // OK, constant index and inside bounds
   a[1] = 3; // OK, constant index and inside bounds
   a[9] = 3; // OK, constant index and inside bounds
   a[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 void g() {
   int a[10];
   for (int i = 0; i < 10; ++i) {
     a[i] = i;
-    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression
     // CHECK-FIXES: gsl::at(a, i) = i;
     gsl::at(a, i) = i; // OK, gsl::at() instead of []
   }
 
   a[-1] = 3; // flagged by clang-diagnostic-array-bounds
   a[10] = 4; // flagged by clang-diagnostic-array-bounds
   a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds