carlosgalvezp created this revision.
Herald added subscribers: shchenz, arphaman, kbarton, kristof.beyls, xazax.hun,
nemanjai.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Currently the fix hint is hardcoded to gsl::at(). This poses
a problem for people who, for a number of reasons, don't want
or cannot use the GSL library.
In these situations, the fix hint does more harm than good
as it creates confusion as to what the fix should be. People
can even misinterpret the fix "gsl::at" as e.g. "std::array::at",
which can lead to even more trouble (e.g. when having guidelines
that disallow exceptions).
Therefore, introduce a new option FixHint that allows users
to specify exactly what the fix hint should be for their
use case. The default is kept to gsl::at.
Since the users can write anything in this option, clang-tidy
should not add fix-it hints in this case, as they could
lead to invalid code. For example, a reasonable fix hint
like "MyCustomSafeArrayWrapper::at()" would not be possible
to replace directly in code, yet it would be of great help
pointing to users in the right direction.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D117205
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-bounds-constant-array-index-fixhint.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-bounds-constant-array-index-fixhint.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-bounds-constant-array-index-fixhint.cpp
@@ -0,0 +1,78 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-constant-array-index %t -- \
+// RUN: -config='{CheckOptions: [{key: cppcoreguidelines-pro-bounds-constant-array-index.FixHint, value: "foo::at()"}]}'
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename T, size_t N>
+struct array {
+ T &operator[](size_t n);
+ T &at(size_t n);
+};
+} // namespace std
+
+namespace gsl {
+template <class T, size_t N>
+T &at(T (&a)[N], size_t index);
+
+template <class T, size_t N>
+T &at(std::array<T, N> &a, size_t index);
+} // namespace gsl
+
+constexpr int const_index(int base) {
+ return base + 3;
+}
+
+void f(std::array<int, 10> a, int pos) {
+ a[pos / 2 /*comment*/] = 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when the index is not an integer constant expression; use foo::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]
+ // CHECK-FIXES-NOT: gsl::at(a, pos / 2 /*comment*/) = 1;
+ int j = a[pos - 1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when the index is not an integer constant expression; use foo::at() instead
+ // CHECK-FIXES-NOT: int j = gsl::at(a, pos - 1);
+
+ a.at(pos - 1) = 2; // OK, at() instead of []
+ gsl::at(a, pos - 1) = 2; // OK, gsl::at() instead of []
+
+ a[-1] = 3;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is negative [cppcoreguidelines-pro-bounds-constant-array-index]
+ a[10] = 4;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements) [cppcoreguidelines-pro-bounds-constant-array-index]
+
+ a[const_index(7)] = 3;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past the end of the array (which contains 10 elements)
+
+ a[0] = 3; // OK, constant index and inside bounds
+ a[1] = 3; // OK, constant index and inside bounds
+ a[9] = 3; // OK, constant index and inside bounds
+ a[const_index(6)] = 3; // OK, constant index and inside bounds
+}
+
+void g() {
+ int a[10];
+ for (int i = 0; i < 10; ++i) {
+ a[i] = i;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript when the index is not an integer constant expression; use foo::at() instead
+ // CHECK-FIXES-NOT: gsl::at(a, i) = i;
+ gsl::at(a, i) = i + 1; // OK, gsl::at() instead of []
+ }
+
+ a[-1] = 3; // flagged by clang-diagnostic-array-bounds
+ a[10] = 4; // flagged by clang-diagnostic-array-bounds
+ a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
+
+ a[0] = 3; // OK, constant index and inside bounds
+ a[1] = 3; // OK, constant index and inside bounds
+ a[9] = 3; // OK, constant index and inside bounds
+ a[const_index(6)] = 3; // OK, constant index and inside bounds
+}
+
+struct S {
+ int &operator[](int i);
+};
+
+void customOperator() {
+ S s;
+ int i = 0;
+ s[i] = 3; // OK, custom operator
+}
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
@@ -23,3 +23,12 @@
A string specifying which include-style is used, `llvm` or `google`. Default
is `llvm`.
+
+.. option:: FixHint
+
+ A string specifying which hint to suggest as a fix, in case users do not want
+ or cannot use GSL. Default is ``gsl::at``. If a non-default fix hint is used,
+ the check will not output fix-it hints, since they could potentially lead to
+ invalid code if the `FixHint` option is not a direct or valid replacement.
+ For example, passing `"MyCustomSafeArrayWrapper::at()"` would be helpful and
+ understandable by the users, but not automatable as a fix-it.
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
@@ -24,6 +24,7 @@
class ProBoundsConstantArrayIndexCheck : public ClangTidyCheck {
const std::string GslHeader;
utils::IncludeInserter Inserter;
+ const std::string FixHint;
public:
ProBoundsConstantArrayIndexCheck(StringRef Name, ClangTidyContext *Context);
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
@@ -22,12 +22,14 @@
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), GslHeader(Options.get("GslHeader", "")),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {}
+ utils::IncludeSorter::IS_LLVM)),
+ FixHint(Options.get("FixHint", "gsl::at()")) {}
void ProBoundsConstantArrayIndexCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "GslHeader", GslHeader);
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
+ Options.store(Opts, "FixHint", FixHint);
}
void ProBoundsConstantArrayIndexCheck::registerPPCallbacks(
@@ -75,10 +77,11 @@
SourceRange IndexRange = IndexExpr->getSourceRange();
auto Diag = diag(Matched->getExprLoc(),
- "do not use array subscript when the index is "
- "not an integer constant expression; use gsl::at() "
- "instead");
- if (!GslHeader.empty()) {
+ std::string("do not use array subscript when the index is "
+ "not an integer constant expression; use ") +
+ FixHint + " instead");
+
+ if (!GslHeader.empty() && (FixHint == "gsl::at()")) {
Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
<< FixItHint::CreateReplacement(
SourceRange(BaseRange.getEnd().getLocWithOffset(1),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
