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

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Thanks for the patch.
A few comments below.


================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:23
@@ +22,3 @@
+
+  Finder->addMatcher(arraySubscriptExpr(hasBase(ignoringImpCasts(hasType(constantArrayType().bind("type")))),
+                                        hasIndex(expr().bind("index"))
----------------
All this code needs to be reformatted to 80 cols.
http://llvm.org/docs/CodingStandards.html#source-code-width

Have you tried clang-format?
It does a great job here.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:28
@@ +27,3 @@
+  Finder->addMatcher(cxxOperatorCallExpr(hasOverloadedOperatorName("[]"),
+                                         hasArgument(0,hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasName("::std::array")).bind("type"))))),
+                                         hasArgument(1, expr().bind("index"))
----------------
    hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasName("::std::array"))))

can be written as

    hasType(cxxRecordDecl(hasName("::std::array")))

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:36
@@ +35,3 @@
+  llvm::APInt ArraySize;
+  const auto *Matched = Result.Nodes.getNodeAs<Expr>("expr");
+  if (const auto *ConstArrayType = Result.Nodes.getNodeAs<ConstantArrayType>("type")) {
----------------
Move this declaration closer to its first use.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:55
@@ +54,3 @@
+  if (!IndexExpr->isIntegerConstantExpr(Index, *Result.Context, nullptr, true)) {
+    //Fixit would need gsl::at()
+    diag(Matched->getExprLoc(), "do not use array subscript when the index is not a compile-time constant; use gsl::at() instead");
----------------
Space after //

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:55
@@ +54,3 @@
+  if (!IndexExpr->isIntegerConstantExpr(Index, *Result.Context, nullptr, true)) {
+    //Fixit would need gsl::at()
+    diag(Matched->getExprLoc(), "do not use array subscript when the index is not a compile-time constant; use gsl::at() instead");
----------------
sbenza wrote:
> Space after //
If this check is enabled, it is very likely that the library is available.
Maybe we should suggest the fix.
You can use clang::tidy::IncludeInserter to add the include if needed.

We could also add a configuration option to enable/disable the automated fix for this case.

================
Comment at: clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h:18
@@ +17,3 @@
+
+/// This checks that all array subscriptions on static arrays and std::arrays have a constant index and are within bounds
+///
----------------
reflow to 80 cols


http://reviews.llvm.org/D13746