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

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.

================
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");
----------------
mgehre wrote:
> sbenza wrote:
> > 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.
> One issue I see is that the gsl header does not have a standard name. I currently know of the microsoft gsl and gsl-lite (https://github.com/martinmoene/gsl-lite), and they don't agree on the name of the header.
> 
> I could make an option "gsl-header-name", which defaults to an empty string. If it is set to an non-empty filename,
> the fix is enabled and that header is used.
> 
> What do you think?
Interesting.
The only downside I see with that is gsl-header-name might be useful for other checks in this package, and the options API only supports per-check options.
You would have to pass it once for every check that needs it.
They are specified as <CheckName>.<OptionName>.
Maybe we can add a Global category or package level flags so that it can apply to all checks under cppcoreguidelines-*.


http://reviews.llvm.org/D13746