[PATCH] D15030: [clang-tidy] add check cppcoreguidelines-pro-bounds-constant-array-index

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:29
@@ +28,3 @@
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "GslHeader", GslHeader);
+}
----------------
Why GslHeader but not IncludeStyle?

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:79
@@ +78,3 @@
+                     "do not use array subscript when the index is "
+                     "not a compile-time constant; use gsl::at() "
+                     "instead");
----------------
instead of "compile-time constant", we should use "integer constant expression".

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:89
@@ +88,3 @@
+
+      auto Insertion = Inserter->CreateIncludeInsertion(
+          Result.SourceManager->getMainFileID(), GslHeader,
----------------
Please do not use auto here, it is not obvious what type will be returned.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:92
@@ +91,3 @@
+          /*IsAngled=*/false);
+      if (Insertion.hasValue())
+        Diag << Insertion.getValue();
----------------
Can just use if (Insertion) instead.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:107
@@ +106,3 @@
+    diag(Matched->getExprLoc(),
+         "std::array<> index %0 is before the beginning of the array")
+        << Index.toString(10);
----------------
"index %0 is negative" is shorter and should be obvious as to what the issue is.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:112
@@ +111,3 @@
+
+  const auto &TemplateArgs = StdArrayDecl->getTemplateArgs();
+  if (TemplateArgs.size() < 2)
----------------
Please do not use auto here either.

================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst:4
@@ +3,3 @@
+
+This check flags all array subscriptions on static arrays and std::arrays that either have a non-compile-time constant index or are out of bounds (for std::array).
+For out-of-bounds checking of static arrays, see the clang-diagnostic-array-bounds check.
----------------
Instead of "array subscriptions" it should be "array subscript expressions". Can also change "non-compile-time constant" to "that either do not have a constant integer expression"

================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst:7
@@ +6,3 @@
+
+Dynamic accesses into arrays are difficult for both tools and humans to validate as safe. gsl::span is a bounds-checked, safe type for accessing arrays of data. gsl::at() is another alternative that ensures single accesses are bounds-checked. If iterators are needed to access an array, use the iterators from an gsl::span constructed over the array.
+
----------------
I'm not comfortable with directly copying the text from the C++ Core Guidelines into our own documentation. I don't believe their license allows this.

================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst:9
@@ +8,3 @@
+
+The check can generated fixes after the option cppcoreguidelines-pro-bounds-constant-array-index.GslHeader has been set to the name of the
+include file that contains gsl::at(), e.g. "gsl/gsl.h".
----------------
s/generated/generate

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-constant-array-index %t -- -config='{CheckOptions: [{key: cppcoreguidelines-pro-bounds-constant-array-index.GslHeader, value: "dir1/gslheader.h"}]}' -- -std=c++11
+// CHECK-FIXES: #include "dir1/gslheader.h"
----------------
We should have an additional test that does not have the GslHeader option set as well.


http://reviews.llvm.org/D15030