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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
