[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

Sun Jan 16 00:14:47 PST 2022

carlosgalvezp added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:84
+
+    if (!GslHeader.empty() && (FixHint == "gsl::at()")) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
----------------
njames93 wrote:
> This restriction on only offering a fix if using `gsl::at` seem a little too restrictive, especially when you factor that the GslHeader is defaulted to an empty string and that is already required for a fix to be emitted.
What would this change for the user? Current behavior is kept, and we can't really predict a good fix if the hint is user-provided. There's many other ways to perform safe indexing than using a free standing function gsl::at-like, and those will require a larger, non-automated refactor.

Another alternative I had in mind is to completely remove the "use gsl::at() instead" hint from the message, and simply warn about the problem (unsafe indexing). CppCoreGuidelines don't actually mention that you need to use gsl::at, at least not from what I can see in the link provided in the docs. 

Removing the fix hint from the warning would solve my concerns (confusing information), and we don't need to introduce extra configuration options.

What do you think?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117205/new/

https://reviews.llvm.org/D117205