-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3 #95220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Add check to replace operator[] with at() Enforce SL.con.3 #95220
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Paul Heidekrüger (PBHDK) ChangesThis PR is based on the PR #90043 by @sebwolf-de, who has given us (@leunam99 and myself) permission to continue his work. The original PR message reads: > The string based test to find out whether the check is applicable on the class is not ideal, but I did not find a more elegant way, yet. As part of the reviews for that PR, @sebwolf-de changed the following:
This PR contains the same changes and, in addition, addresses further feedback provided by the maintainers. Changes in addition to the original PR:
We explicitly don't ignore unused code with We are not sure what the best behaviour for templates is; should we:
@carlosgalvezp and @HerrCai0907 discussed the possibility of disabling the check when exceptions are disabled, but we were unsure whether they'd reached a conclusion and whether it was still relevant when fix-it suggestions are disabled. What do you think? Full diff: https://github.com/llvm/llvm-project/pull/95220.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
index eb35bbc6a538f..fd436859ad04a 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -20,6 +20,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule
NoMallocCheck.cpp
NoSuspendWithLockCheck.cpp
OwningMemoryCheck.cpp
+ PreferAtOverSubscriptOperatorCheck.cpp
PreferMemberInitializerCheck.cpp
ProBoundsArrayToPointerDecayCheck.cpp
ProBoundsConstantArrayIndexCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index e9f0201615616..565a99a865519 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -34,6 +34,7 @@
#include "NoMallocCheck.h"
#include "NoSuspendWithLockCheck.h"
#include "OwningMemoryCheck.h"
+#include "PreferAtOverSubscriptOperatorCheck.h"
#include "PreferMemberInitializerCheck.h"
#include "ProBoundsArrayToPointerDecayCheck.h"
#include "ProBoundsConstantArrayIndexCheck.h"
@@ -102,6 +103,8 @@ class CppCoreGuidelinesModule : public ClangTidyModule {
"cppcoreguidelines-non-private-member-variables-in-classes");
CheckFactories.registerCheck<OwningMemoryCheck>(
"cppcoreguidelines-owning-memory");
+ CheckFactories.registerCheck<PreferAtOverSubscriptOperatorCheck>(
+ "cppcoreguidelines-prefer-at-over-subscript-operator");
CheckFactories.registerCheck<PreferMemberInitializerCheck>(
"cppcoreguidelines-prefer-member-initializer");
CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
new file mode 100644
index 0000000000000..dc036e23e2af1
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
@@ -0,0 +1,124 @@
+//===--- PreferAtOverSubscriptOperatorCheck.cpp - clang-tidy --------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PreferAtOverSubscriptOperatorCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include <algorithm>
+#include <numeric>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+static constexpr std::array<llvm::StringRef, 3> DefaultExclusions = {
+ llvm::StringRef("std::map"), llvm::StringRef("std::unordered_map"),
+ llvm::StringRef("std::flat_map")};
+
+PreferAtOverSubscriptOperatorCheck::PreferAtOverSubscriptOperatorCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {
+
+ ExcludedClasses = clang::tidy::utils::options::parseStringList(
+ Options.get("ExcludeClasses", ""));
+ ExcludedClasses.insert(ExcludedClasses.end(), DefaultExclusions.begin(),
+ DefaultExclusions.end());
+}
+
+void PreferAtOverSubscriptOperatorCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+
+ if (ExcludedClasses.size() == DefaultExclusions.size()) {
+ Options.store(Opts, "ExcludeClasses", "");
+ return;
+ }
+
+ // Sum up the sizes of the defaults ( + semicolons), so we can remove them
+ // from the saved options
+ size_t DefaultsStringLength =
+ std::transform_reduce(DefaultExclusions.begin(), DefaultExclusions.end(),
+ DefaultExclusions.size(), std::plus<>(),
+ [](llvm::StringRef Name) { return Name.size(); });
+
+ std::string Serialized =
+ clang::tidy::utils::options::serializeStringList(ExcludedClasses);
+
+ Options.store(Opts, "ExcludeClasses",
+ Serialized.substr(0, Serialized.size() - DefaultsStringLength));
+}
+
+const CXXMethodDecl *findAlternative(const CXXRecordDecl *MatchedParent,
+ const CXXMethodDecl *MatchedOperator) {
+ for (const CXXMethodDecl *Method : MatchedParent->methods()) {
+ const bool CorrectName = Method->getNameInfo().getAsString() == "at";
+ if (!CorrectName)
+ continue;
+
+ const bool SameReturnType =
+ Method->getReturnType() == MatchedOperator->getReturnType();
+ if (!SameReturnType)
+ continue;
+
+ const bool SameNumberOfArguments =
+ Method->getNumParams() == MatchedOperator->getNumParams();
+ if (!SameNumberOfArguments)
+ continue;
+
+ for (unsigned ArgInd = 0; ArgInd < Method->getNumParams(); ArgInd++) {
+ const bool SameArgType =
+ Method->parameters()[ArgInd]->getOriginalType() ==
+ MatchedOperator->parameters()[ArgInd]->getOriginalType();
+ if (!SameArgType)
+ continue;
+ }
+
+ return Method;
+ }
+ return static_cast<CXXMethodDecl *>(nullptr);
+}
+
+void PreferAtOverSubscriptOperatorCheck::registerMatchers(MatchFinder *Finder) {
+ // Need a callExpr here to match CXXOperatorCallExpr ``(&a)->operator[](0)``
+ // and CXXMemberCallExpr ``a[0]``.
+ Finder->addMatcher(
+ callExpr(
+ callee(
+ cxxMethodDecl(hasOverloadedOperatorName("[]")).bind("operator")),
+ callee(cxxMethodDecl(hasParent(
+ cxxRecordDecl(hasMethod(hasName("at"))).bind("parent")))))
+ .bind("caller"),
+ this);
+}
+
+void PreferAtOverSubscriptOperatorCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const CallExpr *MatchedExpr = Result.Nodes.getNodeAs<CallExpr>("caller");
+ const CXXMethodDecl *MatchedOperator =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("operator");
+ const CXXRecordDecl *MatchedParent =
+ Result.Nodes.getNodeAs<CXXRecordDecl>("parent");
+
+ std::string ClassIdentifier = MatchedParent->getQualifiedNameAsString();
+
+ if (std::find(ExcludedClasses.begin(), ExcludedClasses.end(),
+ ClassIdentifier) != ExcludedClasses.end())
+ return;
+
+ const CXXMethodDecl *Alternative =
+ findAlternative(MatchedParent, MatchedOperator);
+ if (!Alternative)
+ return;
+
+ diag(MatchedExpr->getBeginLoc(),
+ "found possibly unsafe operator[], consider using at() instead");
+ diag(Alternative->getBeginLoc(), "alternative at() defined here",
+ DiagnosticIDs::Note);
+}
+
+} // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
new file mode 100644
index 0000000000000..f2450a7ab3470
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
@@ -0,0 +1,40 @@
+//===--- PreferAtOverSubscriptOperatorCheck.h - clang-tidy ------*- C++ -*-===//
+//===--- PreferMemberInitializerCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Enforce CPP core guidelines SL.con.3
+///
+/// See
+/// https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.html
+class PreferAtOverSubscriptOperatorCheck : public ClangTidyCheck {
+public:
+ PreferAtOverSubscriptOperatorCheck(StringRef Name, ClangTidyContext *Context);
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ // A list of class names that are excluded from the warning
+ std::vector<llvm::StringRef> ExcludedClasses;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PREFERATOVERSUBSCRIPTOPERATORCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6bf70c5cf4f8a..c88e59e453265 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -160,6 +160,11 @@ New checks
Replaces nested ``std::min`` and ``std::max`` calls with an initializer list
where applicable.
+- New :doc:`cppcoreguidelines-prefer-at-over-subscript-operator
+ <clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator>` check.
+
+ Flags the unsafe ``operator[]`` and suggests replacing it with ``at()``.
+
- New :doc:`modernize-use-designated-initializers
<clang-tidy/checks/modernize/use-designated-initializers>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
new file mode 100644
index 0000000000000..42a2100f32582
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - cppcoreguidelines-prefer-at-over-subscript-operator
+
+cppcoreguidelines-prefer-at-over-subscript-operator
+=====================================
+
+This check flags all uses of ``operator[]`` where an equivalent (same parameter and return types) ``at()`` method exists and suggest using that instead.
+
+For example the code
+
+.. code-block:: c++
+ std::array<int, 3> a;
+ int b = a[4];
+
+will generate a warning but
+
+.. code-block:: c++
+ std::unique_ptr<int> a;
+ int b = a[0];
+
+will not.
+
+The classes ``std::map``, ``std::unordered_map`` and ``std::flat_map`` are excluded from this check, because for them the subscript operator has a defined behaviour when a key does not exist (inserting a new element).
+
+Options
+-------
+
+.. option:: ExcludeClasses
+
+ Semicolon-delimited list of class names that should additionally be excluded from this check. By default empty.
+
+This check enforces part of the `SL.con.3 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon3-avoid-bounds-errors>` guideline.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index a698cecc0825c..d83dad31b3547 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -189,6 +189,7 @@ Clang-Tidy Checks
:doc:`cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc>`,
:doc:`cppcoreguidelines-no-suspend-with-lock <cppcoreguidelines/no-suspend-with-lock>`,
:doc:`cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory>`,
+ :doc:`cppcoreguidelines-prefer-at-over-subscript-operator <cppcoreguidelines/prefer-at-over-subscript-operator>`,
:doc:`cppcoreguidelines-prefer-member-initializer <cppcoreguidelines/prefer-member-initializer>`, "Yes"
:doc:`cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines/pro-bounds-array-to-pointer-decay>`,
:doc:`cppcoreguidelines-pro-bounds-constant-array-index <cppcoreguidelines/pro-bounds-constant-array-index>`, "Yes"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp
new file mode 100644
index 0000000000000..cc7088bffeda9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp
@@ -0,0 +1,142 @@
+namespace std {
+ template<typename T, unsigned size>
+ struct array {
+ T operator[](unsigned i) {
+ return T{1};
+ }
+ T at(unsigned i) {
+ return T{1};
+ }
+ };
+
+ template<typename T, typename V>
+ struct map {
+ T operator[](unsigned i) {
+ return T{1};
+ }
+ T at(unsigned i) {
+ return T{1};
+ }
+ };
+
+ template<typename T>
+ struct unique_ptr {
+ T operator[](unsigned i) {
+ return T{1};
+ }
+ };
+
+ template<typename T>
+ struct span {
+ T operator[](unsigned i) {
+ return T{1};
+ }
+ };
+} // namespace std
+
+namespace json {
+ template<typename T>
+ struct node{
+ T operator[](unsigned i) {
+ return T{1};
+ }
+ };
+} // namespace json
+
+struct SubClass : std::array<int, 3> {};
+
+class ExcludedClass1 {
+ public:
+ int operator[](unsigned i) {
+ return 1;
+ }
+ int at(unsigned i) {
+ return 1;
+ }
+};
+
+class ExcludedClass2 {
+ public:
+ int operator[](unsigned i) {
+ return 1;
+ }
+ int at(unsigned i) {
+ return 1;
+ }
+};
+
+
+// RUN: %check_clang_tidy %s \
+// RUN: cppcoreguidelines-prefer-at-over-subscript-operator %t -- \
+// RUN: -config='{CheckOptions: {cppcoreguidelines-prefer-at-over-subscript-operator.ExcludeClasses: "ExcludedClass1;ExcludedClass2"}}'
+std::array<int, 3> a;
+
+auto b = a[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto c = a[1+1];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+constexpr int Index = 1;
+auto d = a[Index];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+int e(int Ind) {
+ return a[Ind];
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+}
+
+auto f = (&a)->operator[](1);
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto g = a.at(0);
+
+std::unique_ptr<int> p;
+auto q = p[0];
+
+std::span<int> s;
+auto t = s[0];
+
+json::node<int> n;
+auto m = n[0];
+
+SubClass Sub;
+auto r = Sub[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+typedef std::array<int, 3> ar;
+ar BehindDef;
+auto u = BehindDef[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+template<typename T> int TestTemplate(T t){
+ return t[0];
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+}
+
+auto v = TestTemplate<>(a);
+auto w = TestTemplate<>(p);
+
+//explicitely excluded classes / struct / template
+ExcludedClass1 E1;
+auto x1 = E1[0];
+
+ExcludedClass2 E2;
+auto x2 = E1[0];
+
+std::map<int,int> TestMap;
+auto y = TestMap[0];
+
+#define SUBSCRIPT_BEHIND_MARCO(x) a[x]
+#define ARG_BEHIND_MACRO 0
+#define OBJECT_BEHIND_MACRO a
+
+auto m1 = SUBSCRIPT_BEHIND_MARCO(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto m2 = a[ARG_BEHIND_MACRO];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
+
+auto m3 = OBJECT_BEHIND_MACRO[0];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: found possibly unsafe operator[], consider using at() instead [cppcoreguidelines-prefer-at-over-subscript-operator]
|
I'm strongly opposed to this, because it's conflating "how to solve the problem" with "what the problem is". If we want to focus on the problem, the check could be named "AvoidSubscriptOperator". This way, the solution to the problem is open for users to decide. I still think "AvoidBoundErrors" is the best name since it maps exactly to what the guidelines call it.
I don't think this is a good idea, as it introduces a dependency towards Clang compiler. And it's a lot more complicated than what I'm proposing. I would appreciate more elaboration as to why this check must use at() and cannot be made optional. |
For me, this sounds to general for what the check currently does. The Enforcement section at the end of the rule SL.con.3 states:
The guidelines do not provide a list of such functions and we are not familliar enough with the standard library to create a comprehensive list ourself, so at this time, we are unable to implement the full rule. |
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
Outdated
Show resolved
Hide resolved
...g-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
Outdated
Show resolved
Hide resolved
...g-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
Outdated
Show resolved
Hide resolved
...g-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
Outdated
Show resolved
Hide resolved
The check certainly doesn't need to fully implement the guideline right now, it can be done in small steps iteratively. I envision that over time the check will fully implement the rule as much as possible. We typically want a 1:1 mapping between a check and a rule. In other words, it would not be good to have N checks implementing different aspects of SL.3, it should be only 1 check. The reason I'm being picky with the name is that it's costly to change it later, it takes 2 clang tidy releases i.e 1 year. |
exception information is in |
I also agree |
I see, that's good. Worth mentioning is that just because "-fno-except" is not present, it does not mean that a project bans exceptions. For example libstdc++/libc++ is typically compiled with exceptions; this won't play well with client code compiled with -fno-except. Even if exceptions were allowed, one can also argue if out-of-bounds is a place that grants usage of exceptions. In some views, exceptions are meant only for truly exceptional situations (e.g. out-of-memory), whereas out-of-bounds is generally seen as a programmer error (contract violation). I don't want to dive deep into such discussion, my point is that error handling is a very debatable topic and also a core design choice to any C++ project, and as such I don't think clang-tidy shouldn't be taking a stance on it. Otherwise some people will be forced to disable a great check (detects violations) due to unwanted fix-it/suggestions (which junior devs may blindly follow without full context). I think clang-tidy could offer an enum option for the fix, like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think the check should propose using at()
. The rule suggests using span
, at()
, range-for
, begin(),end()
or algorithms instead of accessing elements in a container with an unchecked-bounds function.
@carlosgalvezp with AvoidSubscriptOperator
we constrain the check to be about subscript access only, which, admittedly, the check currently is. However, the rule is not constrained to subscript access and the rule has an example with memset
which does not use the subscript operator.
AvoidBoundErrors
sounds a little bit too broad IMO, because the rule talks about containers and is in the SL containers section.
What about: cppcoreguidelines-(avoid-)container-unchecked-bounds(-access)
or
cppcoreguidelines-(avoid-)container-bounds-errors
?
I specifically added container
to both names because the check is in SL.con
.
We are not sure what the best behaviour for templates is
keep the warning and add the name of the class of the object / the template parameters that lead to the message
This may result in a lot of warnings because every diagnostic to the same location will no longer be combined like it is now, and instead we get N
diagnostics (given N
instantiations). On the other hand, these warnings would provide context on the type being used, so I think it is better to have that instead of fewer warnings but without the type named.
We could ignore instantiations and diagnose any use of x[]
inside template declarations. We can then check, if possible, if the type of x
is known to be excluded (e.g., normal type if it is not a dependent type, or TemplateSpecializationType
).
I think clang-tidy could offer an enum option for the fix
+1
That would be a good way to accommodate projects that prefer at()
, because it requires the project to choose to go with this way of solving it (off-by-default).
disabling the check when exceptions are disabled
That would only apply if the check (is enabled to) proposes to use at()
.
...tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-at-over-subscript-operator.cpp
Outdated
Show resolved
Hide resolved
...g-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-at-over-subscript-operator.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
Outdated
Show resolved
Hide resolved
Sounds good to me! My main goal is not constraining the check to only Btw, I realize that this check is part of the bounds profile (Bounds.4), so for consistency it should probably be named |
What do you think of:
That was actually @leunam99's and my main our motivation for getting involved with this work 🙂 |
LGTM! |
Does the fact that this check implements the bounds profile need to be mentioned anywhere else? Or is it enough to have it be implicit, e.g., via the name. |
Not that I'm aware of. It could be worth mentioning in the check documentation though, see for example existing checks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name sounds nice. Thanks.
Please address the concerns here and in #90043 to always suggest at
as well. An option to enable suggesting and providing a fix to use at
(or gsl::at
), like others have suggested, sounds like the best option.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
...a/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.rst
Outdated
Show resolved
Hide resolved
...a/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferAtOverSubscriptOperatorCheck.h
Outdated
Show resolved
Hide resolved
Thank you for taking another look as well as your comments. On it! |
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
...a/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.rst
Outdated
Show resolved
Hide resolved
Nit: consider updating also the commit message / title of this PR to reflect the new behavior of this check (namely, to not enforce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I like check name .
As for changes:
- configuration need a little bit work
- finding .at may need some work (better would be to have this as matcher)
- documentation need minor touch
Overall not bad.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.h
Outdated
Show resolved
Hide resolved
...a/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.rst
Outdated
Show resolved
Hide resolved
...test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-avoid-unchecked-container-accesses.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main issue with check is:
- configuration
- few nits
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsAvoidUncheckedContainerAccesses.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this check is very similar up the cppcoreguidelines-pro-bounds-constant-array-index
check, Is there not a way this could be merged with that check. I'm mindful that people will have both checks enabled and get 2 different warnings for the same violation
…ncheckedContainerAccess.cpp Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
1d87cd0
to
743b42b
Compare
By default, the PR description in GitHub will be the commit message. You can add https://github.blog/news-insights/product-news/commit-together-with-co-authors/ at the end of description. Please use emails, not GitHub names. |
...tra/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access.rst
Outdated
Show resolved
Hide resolved
…-bounds-avoid-unchecked-container-access.rst Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'd give @5chmidti some time for final merge as the main reviewer.
...tra/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access.rst
Outdated
Show resolved
Hide resolved
…-bounds-avoid-unchecked-container-access.rst Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
Last part, could you please make your email public. |
Sorry. Done! |
...tra/docs/clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access.rst
Outdated
Show resolved
Hide resolved
…-bounds-avoid-unchecked-container-access.rst Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
@paulhdk Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Nit: this is not what the check does - it only reports usages of unchecked STL APIs. The user decides (via config options) if it should be replaced with |
@carlosgalvezp: what's the process for updating the commit message? |
We can't alter the commit message after it was merged in |
Yeah we can't change it now, it's ok :) |
Right. Lesson learned! Also, I wanted to thank you both @carlosgalvezp and @vbvictor as well as @EugeneZelenko, @5chmidti, and everyone else for getting this PR across the finish line! |
Thank you for the hard work and patience during the code review process! :) |
This PR is based on the PR #90043 by Sebastian Wolf who has given us (Manuel and myself) permission to continue his work.
The original PR message reads:
As part of the reviews for that PR, Sebastian changed the following:
This PR contains the same changes and, in addition, addresses further feedback provided by the maintainers.
Changes in addition to the original PR:
std::map
,std::flat_map
, andstd::unordered_map
from the analysis by default, and add the ability for users to exclude additional classes from the analysisWe explicitly don't ignore unused code with
TK_IgnoreUnlessSpelledInSource
, as requested by Piotr here, because it caused the template-related tests to fail.We are not sure what the best behaviour for templates is; should we:
at()
will make a different instantiation not compile?Carlos Galvez and Congcong Cai discussed the possibility of disabling the check when exceptions are disabled, but we were unsure whether they'd reached a conclusion and whether it was still relevant when fix-it suggestions are disabled.
What do you think?
Co-authored-by: Manuel Pietsch manuelpietsch@outlook.de
Co-authored-by: Sebastian Wolf wolf.sebastian@in.tum.de