[clang-tools-extra] [clang-tidy] Unsafe CRTP check (PR #82403)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 23 12:14:45 PST 2024
https://github.com/isuckatcs updated https://github.com/llvm/llvm-project/pull/82403
>From a11b863a62f3e27d90e5b337b47d7f20e260d98b Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 16 Feb 2024 00:25:29 +0100
Subject: [PATCH 01/24] added new checker
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 92 +++++++++++++++++++
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 30 ++++++
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../checks/bugprone/unsafe-crtp.rst | 6 ++
.../docs/clang-tidy/checks/list.rst | 1 +
.../checkers/bugprone/unsafe-crtp.cpp | 14 +++
8 files changed, 152 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index a8a23b045f80bb..b7b4d85a86cce2 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -83,6 +83,7 @@
#include "UnhandledExceptionAtNewCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
#include "UniquePtrArrayMismatchCheck.h"
+#include "UnsafeCrtpCheck.h"
#include "UnsafeFunctionsCheck.h"
#include "UnusedLocalNonTrivialVariableCheck.h"
#include "UnusedRaiiCheck.h"
@@ -237,6 +238,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
+ CheckFactories.registerCheck<UnsafeCrtpCheck>(
+ "bugprone-unsafe-crtp");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 1cd6fb207d7625..956b12ad1cdecd 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -79,6 +79,7 @@ add_clang_library(clangTidyBugproneModule
UnhandledExceptionAtNewCheck.cpp
UnhandledSelfAssignmentCheck.cpp
UniquePtrArrayMismatchCheck.cpp
+ UnsafeCrtpCheck.cpp
UnsafeFunctionsCheck.cpp
UnusedLocalNonTrivialVariableCheck.cpp
UnusedRaiiCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
new file mode 100644
index 00000000000000..eb0f6187e58b2e
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -0,0 +1,92 @@
+//===--- UnsafeCrtpCheck.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 "UnsafeCrtpCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+// Finds a node if it's already a bound node.
+AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
+ return Builder->removeBindings(
+ [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+ const auto *BoundRecord = Nodes.getNodeAs<CXXRecordDecl>(ID);
+ return BoundRecord != &Node;
+ });
+}
+} // namespace
+
+void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ cxxRecordDecl(
+ decl().bind("record"),
+ hasAnyBase(hasType(
+ classTemplateSpecializationDecl(
+ hasAnyTemplateArgument(refersToType(recordType(
+ hasDeclaration(cxxRecordDecl(isBoundNode("record")))))))
+ .bind("crtp")))),
+ this);
+}
+
+void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedCRTP = Result.Nodes.getNodeAs<CXXRecordDecl>("crtp");
+
+ MatchedCRTP->dump();
+
+ for (auto &&ctor : MatchedCRTP->ctors()) {
+ if (ctor->getAccess() != AS_private) {
+ ctor->dump();
+ };
+ }
+
+ return;
+
+ // if (!MatchedDecl->hasDefinition())
+ // return;
+
+ // for (auto &&Base : MatchedDecl->bases()) {
+ // const auto *TemplateSpecDecl =
+ // llvm::dyn_cast_if_present<ClassTemplateSpecializationDecl>(
+ // Base.getType()->getAsTagDecl());
+
+ // if (!TemplateSpecDecl)
+ // continue;
+
+ // TemplateSpecDecl->dump();
+
+ // for (auto &&TemplateArg : TemplateSpecDecl->getTemplateArgs().asArray())
+ // {
+ // if (TemplateArg.getKind() != TemplateArgument::Type)
+ // continue;
+
+ // const auto *Record = TemplateArg.getAsType()->getAsCXXRecordDecl();
+
+ // if (Record && Record == MatchedDecl) {
+ // diag(Record->getLocation(), "CRTP found");
+
+ // diag(TemplateSpecDecl->getLocation(), "used as CRTP",
+ // DiagnosticIDs::Note);
+ // TemplateSpecDecl->dump();
+ // }
+ // }
+ // }
+
+ // if (!MatchedDecl->getIdentifier() ||
+ // MatchedDecl->getName().startswith("awesome_"))
+ // return;
+ // diag(MatchedDecl->getLocation(), "function %0 is insufficiently awesome")
+ // << MatchedDecl
+ // << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
+ // diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note);
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
new file mode 100644
index 00000000000000..7c41bbb0678521
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -0,0 +1,30 @@
+//===--- UnsafeCrtpCheck.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_BUGPRONE_UNSAFECRTPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
+class UnsafeCrtpCheck : public ClangTidyCheck {
+public:
+ UnsafeCrtpCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a0b9fcfe0d7774..785e90d0c75eb0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,11 @@ New checks
Replaces certain conditional statements with equivalent calls to
``std::min`` or ``std::max``.
+- New :doc:`bugprone-unsafe-crtp
+ <clang-tidy/checks/bugprone/unsafe-crtp>` check.
+
+ FIXME: add release notes.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
new file mode 100644
index 00000000000000..3352af02a8744f
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - bugprone-unsafe-crtp
+
+bugprone-unsafe-crtp
+====================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 59ef69f390ee91..f33b8122f35945 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -149,6 +149,7 @@ Clang-Tidy Checks
:doc:`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new>`,
:doc:`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment>`,
:doc:`bugprone-unique-ptr-array-mismatch <bugprone/unique-ptr-array-mismatch>`, "Yes"
+ :doc:`bugprone-unsafe-crtp <bugprone/unsafe-crtp>`, "Yes"
:doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
:doc:`bugprone-unused-local-non-trivial-variable <bugprone/unused-local-non-trivial-variable>`,
:doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
new file mode 100644
index 00000000000000..90350ad7abc578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -0,0 +1,14 @@
+// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
+
+// FIXME: Add something that triggers the check here.
+void f();
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [bugprone-unsafe-crtp]
+
+// FIXME: Verify the applied fix.
+// * Make the CHECK patterns specific enough and try to make verified lines
+// unique to avoid incorrect matches.
+// * Use {{}} for regular expressions.
+// CHECK-FIXES: {{^}}void awesome_f();{{$}}
+
+// FIXME: Add something that doesn't trigger the check here.
+void awesome_f2();
>From fed617800b5965e59897dd72bc78f1105cf896f9 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 16 Feb 2024 01:18:56 +0100
Subject: [PATCH 02/24] use a different matcher
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 53 ++++---------------
1 file changed, 9 insertions(+), 44 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index eb0f6187e58b2e..33ad37d7fbbfdb 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -26,15 +26,12 @@ AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(
- cxxRecordDecl(
- decl().bind("record"),
- hasAnyBase(hasType(
- classTemplateSpecializationDecl(
- hasAnyTemplateArgument(refersToType(recordType(
- hasDeclaration(cxxRecordDecl(isBoundNode("record")))))))
- .bind("crtp")))),
- this);
+ Finder->addMatcher(classTemplateSpecializationDecl(
+ decl().bind("crtp"),
+ hasAnyTemplateArgument(refersToType(recordType(
+ hasDeclaration(cxxRecordDecl(isDerivedFrom(
+ cxxRecordDecl(isBoundNode("crtp"))))))))),
+ this);
}
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
@@ -42,44 +39,12 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
MatchedCRTP->dump();
- for (auto &&ctor : MatchedCRTP->ctors()) {
- if (ctor->getAccess() != AS_private) {
- ctor->dump();
+ for (auto &&Ctor : MatchedCRTP->ctors()) {
+ if (Ctor->getAccess() != AS_private) {
+ Ctor->dump();
};
}
- return;
-
- // if (!MatchedDecl->hasDefinition())
- // return;
-
- // for (auto &&Base : MatchedDecl->bases()) {
- // const auto *TemplateSpecDecl =
- // llvm::dyn_cast_if_present<ClassTemplateSpecializationDecl>(
- // Base.getType()->getAsTagDecl());
-
- // if (!TemplateSpecDecl)
- // continue;
-
- // TemplateSpecDecl->dump();
-
- // for (auto &&TemplateArg : TemplateSpecDecl->getTemplateArgs().asArray())
- // {
- // if (TemplateArg.getKind() != TemplateArgument::Type)
- // continue;
-
- // const auto *Record = TemplateArg.getAsType()->getAsCXXRecordDecl();
-
- // if (Record && Record == MatchedDecl) {
- // diag(Record->getLocation(), "CRTP found");
-
- // diag(TemplateSpecDecl->getLocation(), "used as CRTP",
- // DiagnosticIDs::Note);
- // TemplateSpecDecl->dump();
- // }
- // }
- // }
-
// if (!MatchedDecl->getIdentifier() ||
// MatchedDecl->getName().startswith("awesome_"))
// return;
>From 121de31cef02a331a52a4207340be665ce9702dc Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sat, 17 Feb 2024 17:22:59 +0100
Subject: [PATCH 03/24] implemented implicit constructor and friend declaration
logic
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 79 ++++++++++++++++---
1 file changed, 67 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 33ad37d7fbbfdb..9063b385c0d1b4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -9,6 +9,7 @@
#include "UnsafeCrtpCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -23,26 +24,80 @@ AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
return BoundRecord != &Node;
});
}
+
+bool hasPrivateConstructor(const CXXRecordDecl *RD) {
+ for (auto &&Ctor : RD->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ for (auto &&Friend : CRTP->friends()) {
+ if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ return true;
+ }
+
+ return false;
+}
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(classTemplateSpecializationDecl(
- decl().bind("crtp"),
- hasAnyTemplateArgument(refersToType(recordType(
- hasDeclaration(cxxRecordDecl(isDerivedFrom(
- cxxRecordDecl(isBoundNode("crtp"))))))))),
- this);
+ Finder->addMatcher(
+ classTemplateSpecializationDecl(
+ decl().bind("crtp"),
+ hasAnyTemplateArgument(refersToType(recordType(hasDeclaration(
+ cxxRecordDecl(isDerivedFrom(cxxRecordDecl(isBoundNode("crtp"))))
+ .bind("derived")))))),
+ this);
}
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *MatchedCRTP = Result.Nodes.getNodeAs<CXXRecordDecl>("crtp");
- MatchedCRTP->dump();
+ const auto *MatchedCRTP =
+ Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
+ const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+
+ if (!MatchedCRTP->hasUserDeclaredConstructor()) {
+ diag(MatchedCRTP->getLocation(),
+ "the implicit default constructor of the CRTP is publicly accessible")
+ << MatchedCRTP
+ << FixItHint::CreateInsertion(
+ MatchedCRTP->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + MatchedCRTP->getNameAsString() + "() = default;");
+
+ diag(MatchedCRTP->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+
+ // FIXME: Extract this.
+ size_t idx = 0;
+ for (auto &&TemplateArg : MatchedCRTP->getTemplateArgs().asArray()) {
+ if (TemplateArg.getKind() == TemplateArgument::Type &&
+ TemplateArg.getAsType()->getAsCXXRecordDecl() == MatchedDerived) {
+ break;
+ }
+ ++idx;
+ }
- for (auto &&Ctor : MatchedCRTP->ctors()) {
- if (Ctor->getAccess() != AS_private) {
- Ctor->dump();
- };
+ if (hasPrivateConstructor(MatchedCRTP) &&
+ !isDerivedBefriended(MatchedCRTP, MatchedDerived)) {
+ diag(MatchedCRTP->getLocation(),
+ "the CRTP cannot be constructed from the derived class")
+ << MatchedCRTP
+ << FixItHint::CreateInsertion(
+ MatchedCRTP->getBraceRange().getEnd().getLocWithOffset(-1),
+ "friend " +
+ MatchedCRTP->getSpecializedTemplate()
+ ->getTemplateParameters()
+ ->asArray()[idx]
+ ->getNameAsString() +
+ ';');
+ diag(MatchedCRTP->getLocation(),
+ "consider declaring the derived class as friend", DiagnosticIDs::Note);
}
// if (!MatchedDecl->getIdentifier() ||
>From 67ce49a474deb62dcf8393daec8f927235fdc3fe Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 02:18:18 +0100
Subject: [PATCH 04/24] fix friend detection
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 75 +++++++++++--------
1 file changed, 45 insertions(+), 30 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 9063b385c0d1b4..05d9218e9c3a71 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -34,15 +34,39 @@ bool hasPrivateConstructor(const CXXRecordDecl *RD) {
return false;
}
-bool isDerivedBefriended(const CXXRecordDecl *CRTP,
- const CXXRecordDecl *Derived) {
+bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
+ const NamedDecl *Derived) {
for (auto &&Friend : CRTP->friends()) {
- if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
+
+ if (TTPT && TTPT->getDecl() == Derived)
return true;
}
return false;
}
+
+std::optional<const NamedDecl *>
+getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ size_t Idx = 0;
+ bool Found = false;
+ for (auto &&TemplateArg : CRTP->getTemplateArgs().asArray()) {
+ if (TemplateArg.getKind() == TemplateArgument::Type &&
+ TemplateArg.getAsType()->getAsCXXRecordDecl() == Derived) {
+ Found = true;
+ break;
+ }
+ ++Idx;
+ }
+
+ if (!Found)
+ return std::nullopt;
+
+ return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
+}
+
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
@@ -61,42 +85,33 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
- if (!MatchedCRTP->hasUserDeclaredConstructor()) {
- diag(MatchedCRTP->getLocation(),
+ const auto *CRTPTemplate =
+ MatchedCRTP->getSpecializedTemplate()->getTemplatedDecl();
+
+ if (!CRTPTemplate->hasUserDeclaredConstructor()) {
+ diag(CRTPTemplate->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
- << MatchedCRTP
+ << CRTPTemplate
<< FixItHint::CreateInsertion(
- MatchedCRTP->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + MatchedCRTP->getNameAsString() + "() = default;");
+ CRTPTemplate->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + CRTPTemplate->getNameAsString() + "() = default;");
- diag(MatchedCRTP->getLocation(), "consider making it private",
+ diag(CRTPTemplate->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
- // FIXME: Extract this.
- size_t idx = 0;
- for (auto &&TemplateArg : MatchedCRTP->getTemplateArgs().asArray()) {
- if (TemplateArg.getKind() == TemplateArgument::Type &&
- TemplateArg.getAsType()->getAsCXXRecordDecl() == MatchedDerived) {
- break;
- }
- ++idx;
- }
+ const auto *DerivedTemplateParameter =
+ *getDerivedParameter(MatchedCRTP, MatchedDerived);
- if (hasPrivateConstructor(MatchedCRTP) &&
- !isDerivedBefriended(MatchedCRTP, MatchedDerived)) {
- diag(MatchedCRTP->getLocation(),
+ if (hasPrivateConstructor(CRTPTemplate) &&
+ !isDerivedParameterBefriended(CRTPTemplate, DerivedTemplateParameter)) {
+ diag(CRTPTemplate->getLocation(),
"the CRTP cannot be constructed from the derived class")
- << MatchedCRTP
+ << CRTPTemplate
<< FixItHint::CreateInsertion(
- MatchedCRTP->getBraceRange().getEnd().getLocWithOffset(-1),
- "friend " +
- MatchedCRTP->getSpecializedTemplate()
- ->getTemplateParameters()
- ->asArray()[idx]
- ->getNameAsString() +
- ';');
- diag(MatchedCRTP->getLocation(),
+ CRTPTemplate->getBraceRange().getEnd().getLocWithOffset(-1),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';');
+ diag(CRTPTemplate->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
>From 77fc451bb00d240b3b19857d7ae418358a9cf3d4 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 02:28:22 +0100
Subject: [PATCH 05/24] cleanup
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 48 ++++++++-----------
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 05d9218e9c3a71..09d2db4d993984 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -7,9 +7,7 @@
//===----------------------------------------------------------------------===//
#include "UnsafeCrtpCheck.h"
-#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -80,48 +78,40 @@ void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
}
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
-
- const auto *MatchedCRTP =
+ const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
- const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
- const auto *CRTPTemplate =
- MatchedCRTP->getSpecializedTemplate()->getTemplatedDecl();
+ const CXXRecordDecl *CRTPDeclaration =
+ CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
- if (!CRTPTemplate->hasUserDeclaredConstructor()) {
- diag(CRTPTemplate->getLocation(),
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ diag(CRTPDeclaration->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
- << CRTPTemplate
+ << CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPTemplate->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + CRTPTemplate->getNameAsString() + "() = default;");
-
- diag(CRTPTemplate->getLocation(), "consider making it private",
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + CRTPDeclaration->getNameAsString() +
+ "() = default;");
+ diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
const auto *DerivedTemplateParameter =
- *getDerivedParameter(MatchedCRTP, MatchedDerived);
+ *getDerivedParameter(CRTPInstantiation, Derived);
- if (hasPrivateConstructor(CRTPTemplate) &&
- !isDerivedParameterBefriended(CRTPTemplate, DerivedTemplateParameter)) {
- diag(CRTPTemplate->getLocation(),
+ if (hasPrivateConstructor(CRTPDeclaration) &&
+ !isDerivedParameterBefriended(CRTPDeclaration,
+ DerivedTemplateParameter)) {
+ diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
- << CRTPTemplate
+ << CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPTemplate->getBraceRange().getEnd().getLocWithOffset(-1),
+ CRTPDeclaration->getBraceRange().getEnd().getLocWithOffset(-1),
"friend " + DerivedTemplateParameter->getNameAsString() + ';');
- diag(CRTPTemplate->getLocation(),
+ diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
-
- // if (!MatchedDecl->getIdentifier() ||
- // MatchedDecl->getName().startswith("awesome_"))
- // return;
- // diag(MatchedDecl->getLocation(), "function %0 is insufficiently awesome")
- // << MatchedDecl
- // << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
- // diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note);
}
} // namespace clang::tidy::bugprone
>From f486bb6ad5c3308c805fd3121278cc344edccc63 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 04:17:25 +0100
Subject: [PATCH 06/24] implement ctor checks
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 09d2db4d993984..85d8b8a8ab8cd8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "UnsafeCrtpCheck.h"
+#include "../utils/LexerUtils.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
using namespace clang::ast_matchers;
@@ -65,6 +66,24 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
}
+std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
+ std::string OriginalAccess,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ std::vector<FixItHint> Hints;
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->isExplicitlyDefaulted()
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ .getLocWithOffset(1)
+ : Ctor->getEndLoc().getLocWithOffset(1),
+ '\n' + OriginalAccess + ':' + '\n'));
+
+ return Hints;
+}
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
@@ -112,6 +131,23 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
+
+ for (auto &&Ctor : CRTPDeclaration->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ continue;
+
+ bool IsPublic = Ctor->getAccess() == AS_public;
+ std::string Access = IsPublic ? "public" : "protected";
+
+ diag(Ctor->getLocation(),
+ "%0 contructor allows the CRTP to be %select{inherited "
+ "from|constructed}1 as a regular template class")
+ << Access << IsPublic << Ctor
+ << hintMakeCtorPrivate(Ctor, Access, *Result.SourceManager,
+ getLangOpts());
+ diag(Ctor->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
}
} // namespace clang::tidy::bugprone
>From dc250cdebd9c990ffb16ffde0e2b2794597e1647 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 04:23:18 +0100
Subject: [PATCH 07/24] fix struct default ctor generation
---
clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 85d8b8a8ab8cd8..9295ba0009ac24 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -111,7 +111,8 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
<< FixItHint::CreateInsertion(
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
"private: " + CRTPDeclaration->getNameAsString() +
- "() = default;");
+ "() = default;" +
+ (CRTPDeclaration->isStruct() ? "\npublic:\n" : ""));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
>From 10355cb112d1ec8b07c1a016f7cf6e327c30cc94 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 18:09:47 +0100
Subject: [PATCH 08/24] testing
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 11 +-
.../checkers/bugprone/unsafe-crtp.cpp | 172 ++++++++++++++++--
2 files changed, 166 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 9295ba0009ac24..b97e0142420be4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -110,9 +110,9 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
<< CRTPDeclaration
<< FixItHint::CreateInsertion(
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + CRTPDeclaration->getNameAsString() +
- "() = default;" +
- (CRTPDeclaration->isStruct() ? "\npublic:\n" : ""));
+ (CRTPDeclaration->isStruct() ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;" +
+ (CRTPDeclaration->isStruct() ? "\npublic:\n" : "\n"));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
@@ -127,8 +127,9 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
"the CRTP cannot be constructed from the derived class")
<< CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPDeclaration->getBraceRange().getEnd().getLocWithOffset(-1),
- "friend " + DerivedTemplateParameter->getNameAsString() + ';');
+ CRTPDeclaration->getBraceRange().getEnd(),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';' +
+ '\n');
diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
index 90350ad7abc578..b8e38aa18e5e41 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -1,14 +1,162 @@
// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
-// FIXME: Add something that triggers the check here.
-void f();
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [bugprone-unsafe-crtp]
-
-// FIXME: Verify the applied fix.
-// * Make the CHECK patterns specific enough and try to make verified lines
-// unique to avoid incorrect matches.
-// * Use {{}} for regular expressions.
-// CHECK-FIXES: {{^}}void awesome_f();{{$}}
-
-// FIXME: Add something that doesn't trigger the check here.
-void awesome_f2();
+namespace class_implicit_ctor {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+class A : CRTP<A> {};
+} // namespace class_implicit_ctor
+
+namespace class_uncostructible {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+} // namespace class_uncostructible
+
+namespace class_public_default_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_default_ctor
+
+namespace class_public_user_provided_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_user_provided_ctor
+
+namespace class_public_multiple_user_provided_ctors {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_multiple_user_provided_ctors
+
+namespace class_protected_ctors {
+template <typename T>
+class CRTP {
+protected:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}protected:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}protected:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}protected:
+};
+
+class A : CRTP<A> {};
+} // namespace class_protected_ctors
+
+namespace struct_implicit_ctor {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A> {};
+} // namespace struct_implicit_ctor
+
+namespace struct_default_ctor {
+template <typename T>
+struct CRTP {
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace struct_default_ctor
+
+namespace same_class_multiple_crtps {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+template <typename T>
+struct CRTP2 {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP2() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A>, CRTP2<A> {};
+} // namespace same_class_multiple_crtps
+
+namespace same_crtp_multiple_classes {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+class B : CRTP<B> {};
+} // namespace same_crtp_multiple_classes
+
+namespace crtp_template {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend U;
+ CRTP() = default;
+};
+
+class A : CRTP<int, A> {};
+} // namespace crtp_template
+
+namespace crtp_template2 {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A, A> {};
+} // namespace crtp_template2
>From d0e20bb3854fa7dc3331adf7a9da7dcf26d759a6 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 15:53:16 +0100
Subject: [PATCH 09/24] docs
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 2 +-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checks/bugprone/unsafe-crtp.rst | 58 ++++++++++++++++++-
3 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
index 7c41bbb0678521..ac1a8f09208f95 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -13,7 +13,7 @@
namespace clang::tidy::bugprone {
-/// FIXME: Write a short description.
+/// Finds CRTP used in an error-prone way.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 785e90d0c75eb0..4883e1bcaad35c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -109,7 +109,7 @@ New checks
- New :doc:`bugprone-unsafe-crtp
<clang-tidy/checks/bugprone/unsafe-crtp>` check.
- FIXME: add release notes.
+ Detects error-prone CRTP.
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
index 3352af02a8744f..8e6955999512a6 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -3,4 +3,60 @@
bugprone-unsafe-crtp
====================
-FIXME: Describe what patterns does the check detect and why. Give examples.
+Finds CRTP used in an error-prone way.
+
+If the constructor of a class intended to be used in a CRTP is public, then
+it allows users to construct that class on its own.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ public:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ CRTP<int> BadInstance;
+
+If the constructor is protected, the possibility of an accidental instantiation
+is prevented, however it can fade an error, when a different class is used as
+the template parameter instead of the derived one.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ protected:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad BadInstance;
+
+To ensure that no accidental instantiation happens, the best practice is to make
+the constructor private and declare the derived class as friend.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ CRTP() = default;
+ friend T;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad CompileTimeError;
+
+ CRTP<int> AlsoCompileTimeError;
>From aff328b35303a1a366625d7764f08431b8329f8b Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 18:36:19 +0100
Subject: [PATCH 10/24] cleanup
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 19 ++++++++++---------
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index b97e0142420be4..a1caf934ef2cd6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -67,7 +67,7 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
}
std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
- std::string OriginalAccess,
+ const std::string &OriginalAccess,
const SourceManager &SM,
const LangOptions &LangOpts) {
std::vector<FixItHint> Hints;
@@ -75,12 +75,12 @@ std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
Hints.emplace_back(FixItHint::CreateInsertion(
Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
- Hints.emplace_back(FixItHint::CreateInsertion(
+ SourceLocation CtorEndLoc =
Ctor->isExplicitlyDefaulted()
? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
- .getLocWithOffset(1)
- : Ctor->getEndLoc().getLocWithOffset(1),
- '\n' + OriginalAccess + ':' + '\n'));
+ : Ctor->getEndLoc();
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
return Hints;
}
@@ -100,19 +100,20 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
-
const CXXRecordDecl *CRTPDeclaration =
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ bool IsStruct = CRTPDeclaration->isStruct();
+
diag(CRTPDeclaration->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
<< CRTPDeclaration
<< FixItHint::CreateInsertion(
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
- (CRTPDeclaration->isStruct() ? "\nprivate:\n" : "\n") +
- CRTPDeclaration->getNameAsString() + "() = default;" +
- (CRTPDeclaration->isStruct() ? "\npublic:\n" : "\n"));
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : ""));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4883e1bcaad35c..c80746dce2de25 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -109,7 +109,7 @@ New checks
- New :doc:`bugprone-unsafe-crtp
<clang-tidy/checks/bugprone/unsafe-crtp>` check.
- Detects error-prone CRTP.
+ Detects error-prone CRTP usage.
New check aliases
^^^^^^^^^^^^^^^^^
>From d5c77a42b4174ea44b33c76411b3f308508325ef Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 18:50:41 +0100
Subject: [PATCH 11/24] more testing
---
.../checkers/bugprone/unsafe-crtp.cpp | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
index b8e38aa18e5e41..13bd19610ed942 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -160,3 +160,45 @@ class CRTP {
class A : CRTP<A, A> {};
} // namespace crtp_template2
+
+namespace template_derived {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+// FIXME: Ideally the warning should be triggered without instantiation.
+void foo() {
+ A<int> A;
+ (void) A;
+}
+} // namespace template_derived
+
+namespace template_derived_explicit_specialization {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+template<>
+class A<int> : CRTP<A<int>> {};
+} // namespace template_derived_explicit_specialization
+
+namespace no_warning {
+template <typename T>
+class CRTP
+{
+ CRTP() = default;
+ friend T;
+};
+
+class A : CRTP<A> {};
+} // namespace no_warning
>From 9901bc3af91afe26f0f54382fdffc83f97164541 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 19:46:13 +0100
Subject: [PATCH 12/24] extend
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 21 ++++++++++----
.../checkers/bugprone/unsafe-crtp.cpp | 28 +++++++++++++++++++
2 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index a1caf934ef2cd6..7bb50aaf950cbe 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -34,12 +34,22 @@ bool hasPrivateConstructor(const CXXRecordDecl *RD) {
}
bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
- const NamedDecl *Derived) {
+ const NamedDecl *Param) {
for (auto &&Friend : CRTP->friends()) {
const auto *TTPT =
dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
- if (TTPT && TTPT->getDecl() == Derived)
+ if (TTPT && TTPT->getDecl() == Param)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ for (auto &&Friend : CRTP->friends()) {
+ if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
return true;
}
@@ -99,7 +109,7 @@ void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
- const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
const CXXRecordDecl *CRTPDeclaration =
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
@@ -119,11 +129,12 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
}
const auto *DerivedTemplateParameter =
- *getDerivedParameter(CRTPInstantiation, Derived);
+ *getDerivedParameter(CRTPInstantiation, DerivedRecord);
if (hasPrivateConstructor(CRTPDeclaration) &&
!isDerivedParameterBefriended(CRTPDeclaration,
- DerivedTemplateParameter)) {
+ DerivedTemplateParameter) &&
+ !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord)) {
diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
<< CRTPDeclaration
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
index 13bd19610ed942..edcfd67677fd7b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -192,6 +192,34 @@ template<>
class A<int> : CRTP<A<int>> {};
} // namespace template_derived_explicit_specialization
+namespace explicit_derived_friend {
+class A;
+
+template <typename T>
+class CRTP {
+ CRTP() = default;
+ friend A;
+};
+
+class A : CRTP<A> {};
+} // namespace explicit_derived_friend
+
+namespace explicit_derived_friend_multiple {
+class A;
+
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+ friend A;
+};
+
+class A : CRTP<A> {};
+class B : CRTP<B> {};
+} // namespace explicit_derived_friend_multiple
+
namespace no_warning {
template <typename T>
class CRTP
>From 74f69781f6a8881da59cb6e52ca3a2230d2cffc0 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 20:09:12 +0100
Subject: [PATCH 13/24] format
---
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index b7b4d85a86cce2..78e128fb0e5e6d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -238,8 +238,7 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
- CheckFactories.registerCheck<UnsafeCrtpCheck>(
- "bugprone-unsafe-crtp");
+ CheckFactories.registerCheck<UnsafeCrtpCheck>("bugprone-unsafe-crtp");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(
>From fe8dbf3f9cc0cd294ed2b3e1148bdbff77009847 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 20:29:39 +0100
Subject: [PATCH 14/24] addressed comments
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 12 ++++++++----
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 3 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 10 +++++-----
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 7bb50aaf950cbe..227bd9803a1267 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -85,7 +85,7 @@ std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
Hints.emplace_back(FixItHint::CreateInsertion(
Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
- SourceLocation CtorEndLoc =
+ const SourceLocation CtorEndLoc =
Ctor->isExplicitlyDefaulted()
? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
: Ctor->getEndLoc();
@@ -114,7 +114,7 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
- bool IsStruct = CRTPDeclaration->isStruct();
+ const bool IsStruct = CRTPDeclaration->isStruct();
diag(CRTPDeclaration->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
@@ -150,8 +150,8 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
if (Ctor->getAccess() == AS_private)
continue;
- bool IsPublic = Ctor->getAccess() == AS_public;
- std::string Access = IsPublic ? "public" : "protected";
+ const bool IsPublic = Ctor->getAccess() == AS_public;
+ const std::string Access = IsPublic ? "public" : "protected";
diag(Ctor->getLocation(),
"%0 contructor allows the CRTP to be %select{inherited "
@@ -164,4 +164,8 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
}
}
+bool UnsafeCrtpCheck::isLanguageVersionSupported(
+ const LangOptions &LangOpts) const {
+ return LangOpts.CPlusPlus;
+}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
index ac1a8f09208f95..fb7245edeb0fa9 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -13,7 +13,7 @@
namespace clang::tidy::bugprone {
-/// Finds CRTP used in an error-prone way.
+/// Detects error-prone CRTP usage.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
@@ -23,6 +23,7 @@ class UnsafeCrtpCheck : public ClangTidyCheck {
: ClangTidyCheck(Name, Context) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index c80746dce2de25..bfe4e5761bbe8e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,17 +100,17 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`bugprone-unsafe-crtp
+ <clang-tidy/checks/bugprone/unsafe-crtp>` check.
+
+ Detects error-prone CRTP usage.
+
- New :doc:`readability-use-std-min-max
<clang-tidy/checks/readability/use-std-min-max>` check.
Replaces certain conditional statements with equivalent calls to
``std::min`` or ``std::max``.
-- New :doc:`bugprone-unsafe-crtp
- <clang-tidy/checks/bugprone/unsafe-crtp>` check.
-
- Detects error-prone CRTP usage.
-
New check aliases
^^^^^^^^^^^^^^^^^
>From a7521e608ec71e356964575381da9d5604ca0d07 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 00:24:41 +0100
Subject: [PATCH 15/24] comments
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 34 +++++++------------
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 3 +-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checkers/bugprone/unsafe-crtp.cpp | 2 +-
4 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 227bd9803a1267..bdbb2ab7da35e5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -14,17 +14,7 @@ using namespace clang::ast_matchers;
namespace clang::tidy::bugprone {
-namespace {
-// Finds a node if it's already a bound node.
-AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
- return Builder->removeBindings(
- [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
- const auto *BoundRecord = Nodes.getNodeAs<CXXRecordDecl>(ID);
- return BoundRecord != &Node;
- });
-}
-
-bool hasPrivateConstructor(const CXXRecordDecl *RD) {
+static bool hasPrivateConstructor(const CXXRecordDecl *RD) {
for (auto &&Ctor : RD->ctors()) {
if (Ctor->getAccess() == AS_private)
return true;
@@ -33,8 +23,8 @@ bool hasPrivateConstructor(const CXXRecordDecl *RD) {
return false;
}
-bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
- const NamedDecl *Param) {
+static bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
+ const NamedDecl *Param) {
for (auto &&Friend : CRTP->friends()) {
const auto *TTPT =
dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
@@ -46,8 +36,8 @@ bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
return false;
}
-bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
- const CXXRecordDecl *Derived) {
+static bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
for (auto &&Friend : CRTP->friends()) {
if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
return true;
@@ -56,7 +46,7 @@ bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
return false;
}
-std::optional<const NamedDecl *>
+static std::optional<const NamedDecl *>
getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
const CXXRecordDecl *Derived) {
size_t Idx = 0;
@@ -76,10 +66,10 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
}
-std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
- const std::string &OriginalAccess,
- const SourceManager &SM,
- const LangOptions &LangOpts) {
+static std::vector<FixItHint>
+hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
+ const std::string &OriginalAccess, const SourceManager &SM,
+ const LangOptions &LangOpts) {
std::vector<FixItHint> Hints;
Hints.emplace_back(FixItHint::CreateInsertion(
@@ -94,14 +84,14 @@ std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
return Hints;
}
-} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
classTemplateSpecializationDecl(
decl().bind("crtp"),
hasAnyTemplateArgument(refersToType(recordType(hasDeclaration(
- cxxRecordDecl(isDerivedFrom(cxxRecordDecl(isBoundNode("crtp"))))
+ cxxRecordDecl(
+ isDerivedFrom(cxxRecordDecl(equalsBoundNode("crtp"))))
.bind("derived")))))),
this);
}
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
index fb7245edeb0fa9..96f252d6707dc9 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -13,7 +13,8 @@
namespace clang::tidy::bugprone {
-/// Detects error-prone CRTP usage.
+/// Detects error-prone CRTP usage, when the CRTP can be constructed outside
+/// itself and the derived class.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index bfe4e5761bbe8e..b9f88df2944e19 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -103,7 +103,7 @@ New checks
- New :doc:`bugprone-unsafe-crtp
<clang-tidy/checks/bugprone/unsafe-crtp>` check.
- Detects error-prone CRTP usage.
+ Detects error-prone CRTP usage, when the CRTP can be constructed outside itself and the derived class.
- New :doc:`readability-use-std-min-max
<clang-tidy/checks/readability/use-std-min-max>` check.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
index edcfd67677fd7b..f3dc9ea46d0ec9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
+// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t -- -- -fno-delayed-template-parsing
namespace class_implicit_ctor {
template <typename T>
>From c8cfa144f98df51b52319b60ad6f709333f96408 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 00:29:08 +0100
Subject: [PATCH 16/24] rename check
---
.../bugprone/BugproneTidyModule.cpp | 4 +-
.../clang-tidy/bugprone/CMakeLists.txt | 2 +-
... => CrtpConstructorAccessibilityCheck.cpp} | 10 ++---
....h => CrtpConstructorAccessibilityCheck.h} | 14 +++----
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
...rst => crtp-constructor-accessibility.rst} | 6 +--
.../docs/clang-tidy/checks/list.rst | 2 +-
...cpp => crtp-constructor-accessibility.cpp} | 40 +++++++++----------
8 files changed, 41 insertions(+), 41 deletions(-)
rename clang-tools-extra/clang-tidy/bugprone/{UnsafeCrtpCheck.cpp => CrtpConstructorAccessibilityCheck.cpp} (93%)
rename clang-tools-extra/clang-tidy/bugprone/{UnsafeCrtpCheck.h => CrtpConstructorAccessibilityCheck.h} (59%)
rename clang-tools-extra/docs/clang-tidy/checks/bugprone/{unsafe-crtp.rst => crtp-constructor-accessibility.rst} (89%)
rename clang-tools-extra/test/clang-tidy/checkers/bugprone/{unsafe-crtp.cpp => crtp-constructor-accessibility.cpp} (84%)
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 78e128fb0e5e6d..0e2957ee21a791 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -83,7 +83,7 @@
#include "UnhandledExceptionAtNewCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
#include "UniquePtrArrayMismatchCheck.h"
-#include "UnsafeCrtpCheck.h"
+#include "CrtpConstructorAccessibilityCheck.h"
#include "UnsafeFunctionsCheck.h"
#include "UnusedLocalNonTrivialVariableCheck.h"
#include "UnusedRaiiCheck.h"
@@ -238,7 +238,7 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
- CheckFactories.registerCheck<UnsafeCrtpCheck>("bugprone-unsafe-crtp");
+ CheckFactories.registerCheck<CrtpConstructorAccessibilityCheck>("bugprone-crtp-constructor-accessibility");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 956b12ad1cdecd..638fba03a43587 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -79,7 +79,7 @@ add_clang_library(clangTidyBugproneModule
UnhandledExceptionAtNewCheck.cpp
UnhandledSelfAssignmentCheck.cpp
UniquePtrArrayMismatchCheck.cpp
- UnsafeCrtpCheck.cpp
+ CrtpConstructorAccessibilityCheck.cpp
UnsafeFunctionsCheck.cpp
UnusedLocalNonTrivialVariableCheck.cpp
UnusedRaiiCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
similarity index 93%
rename from clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
rename to clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index bdbb2ab7da35e5..8c78d293239c9e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -1,4 +1,4 @@
-//===--- UnsafeCrtpCheck.cpp - clang-tidy ---------------------------------===//
+//===--- CrtpConstructorAccessibilityCheck.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.
@@ -6,7 +6,7 @@
//
//===----------------------------------------------------------------------===//
-#include "UnsafeCrtpCheck.h"
+#include "CrtpConstructorAccessibilityCheck.h"
#include "../utils/LexerUtils.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
@@ -85,7 +85,7 @@ hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
return Hints;
}
-void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
+void CrtpConstructorAccessibilityCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
classTemplateSpecializationDecl(
decl().bind("crtp"),
@@ -96,7 +96,7 @@ void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
this);
}
-void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
+void CrtpConstructorAccessibilityCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
@@ -154,7 +154,7 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
}
}
-bool UnsafeCrtpCheck::isLanguageVersionSupported(
+bool CrtpConstructorAccessibilityCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
return LangOpts.CPlusPlus;
}
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.h
similarity index 59%
rename from clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
rename to clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.h
index 96f252d6707dc9..cf267c756bd041 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.h
@@ -1,4 +1,4 @@
-//===--- UnsafeCrtpCheck.h - clang-tidy -------------------------*- C++ -*-===//
+//===--- CrtpConstructorAccessibilityCheck.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.
@@ -6,8 +6,8 @@
//
//===----------------------------------------------------------------------===//
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H
#include "../ClangTidyCheck.h"
@@ -17,10 +17,10 @@ namespace clang::tidy::bugprone {
/// itself and the derived class.
///
/// For the user-facing documentation see:
-/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
-class UnsafeCrtpCheck : public ClangTidyCheck {
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/crtp-constructor-accessibility.html
+class CrtpConstructorAccessibilityCheck : public ClangTidyCheck {
public:
- UnsafeCrtpCheck(StringRef Name, ClangTidyContext *Context)
+ CrtpConstructorAccessibilityCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
@@ -29,4 +29,4 @@ class UnsafeCrtpCheck : public ClangTidyCheck {
} // namespace clang::tidy::bugprone
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b9f88df2944e19..149e314915a0d6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,8 +100,8 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
-- New :doc:`bugprone-unsafe-crtp
- <clang-tidy/checks/bugprone/unsafe-crtp>` check.
+- New :doc:`bugprone-crtp-constructor-accessibility
+ <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` check.
Detects error-prone CRTP usage, when the CRTP can be constructed outside itself and the derived class.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
similarity index 89%
rename from clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
rename to clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
index 8e6955999512a6..cbbffeafcf90fc 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
@@ -1,7 +1,7 @@
-.. title:: clang-tidy - bugprone-unsafe-crtp
+.. title:: clang-tidy - bugprone-crtp-constructor-accessibility
-bugprone-unsafe-crtp
-====================
+bugprone-crtp-constructor-accessibility
+=======================================
Finds CRTP used in an error-prone way.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index f33b8122f35945..391570ce3fa6b7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@ Clang-Tidy Checks
:doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
:doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,
:doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
+ :doc:`bugprone-crtp-constructor-accessibility <bugprone/crtp-constructor-accessibility>`, "Yes"
:doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
:doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`,
:doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`,
@@ -149,7 +150,6 @@ Clang-Tidy Checks
:doc:`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new>`,
:doc:`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment>`,
:doc:`bugprone-unique-ptr-array-mismatch <bugprone/unique-ptr-array-mismatch>`, "Yes"
- :doc:`bugprone-unsafe-crtp <bugprone/unsafe-crtp>`, "Yes"
:doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
:doc:`bugprone-unused-local-non-trivial-variable <bugprone/unused-local-non-trivial-variable>`,
:doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
similarity index 84%
rename from clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index f3dc9ea46d0ec9..e4efd01007c9dd 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -1,9 +1,9 @@
-// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing
namespace class_implicit_ctor {
template <typename T>
class CRTP {};
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
// CHECK-FIXES: CRTP() = default;
@@ -13,7 +13,7 @@ class A : CRTP<A> {};
namespace class_uncostructible {
template <typename T>
class CRTP {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
// CHECK-FIXES: friend T;
CRTP() = default;
@@ -27,7 +27,7 @@ template <typename T>
class CRTP {
public:
CRTP() = default;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
};
@@ -40,7 +40,7 @@ template <typename T>
class CRTP {
public:
CRTP(int) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
};
@@ -53,11 +53,11 @@ template <typename T>
class CRTP {
public:
CRTP(int) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
CRTP(float) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}public:
};
@@ -70,15 +70,15 @@ template <typename T>
class CRTP {
protected:
CRTP(int) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}protected:
CRTP() = default;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}protected:
CRTP(float) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}protected:
};
@@ -89,7 +89,7 @@ class A : CRTP<A> {};
namespace struct_implicit_ctor {
template <typename T>
struct CRTP {};
-// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
@@ -100,7 +100,7 @@ namespace struct_default_ctor {
template <typename T>
struct CRTP {
CRTP() = default;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
};
@@ -111,13 +111,13 @@ class A : CRTP<A> {};
namespace same_class_multiple_crtps {
template <typename T>
struct CRTP {};
-// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
template <typename T>
struct CRTP2 {};
-// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP2() = default;{{[[:space:]]*}}public:
@@ -127,7 +127,7 @@ class A : CRTP<A>, CRTP2<A> {};
namespace same_crtp_multiple_classes {
template <typename T>
class CRTP {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
// CHECK-FIXES: friend T;
CRTP() = default;
@@ -140,7 +140,7 @@ class B : CRTP<B> {};
namespace crtp_template {
template <typename T, typename U>
class CRTP {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
// CHECK-FIXES: friend U;
CRTP() = default;
@@ -152,7 +152,7 @@ class A : CRTP<int, A> {};
namespace crtp_template2 {
template <typename T, typename U>
class CRTP {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
// CHECK-FIXES: friend T;
CRTP() = default;
@@ -164,7 +164,7 @@ class A : CRTP<A, A> {};
namespace template_derived {
template <typename T>
class CRTP {};
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
// CHECK-FIXES: CRTP() = default;
@@ -181,7 +181,7 @@ void foo() {
namespace template_derived_explicit_specialization {
template <typename T>
class CRTP {};
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
// CHECK-FIXES: CRTP() = default;
@@ -209,7 +209,7 @@ class A;
template <typename T>
class CRTP {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
// CHECK-FIXES: friend T;
CRTP() = default;
>From 4fb78b2db424cf55368d9c0a993b4c36fcfe3958 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 01:16:53 +0100
Subject: [PATCH 17/24] comments
---
.../bugprone/CrtpConstructorAccessibilityCheck.cpp | 8 +++++---
.../checks/bugprone/crtp-constructor-accessibility.rst | 3 ++-
.../checkers/bugprone/crtp-constructor-accessibility.cpp | 2 +-
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index 8c78d293239c9e..86f291a83b1875 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -1,4 +1,5 @@
-//===--- CrtpConstructorAccessibilityCheck.cpp - clang-tidy ---------------------------------===//
+//===--- CrtpConstructorAccessibilityCheck.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.
@@ -96,7 +97,8 @@ void CrtpConstructorAccessibilityCheck::registerMatchers(MatchFinder *Finder) {
this);
}
-void CrtpConstructorAccessibilityCheck::check(const MatchFinder::MatchResult &Result) {
+void CrtpConstructorAccessibilityCheck::check(
+ const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
@@ -156,6 +158,6 @@ void CrtpConstructorAccessibilityCheck::check(const MatchFinder::MatchResult &Re
bool CrtpConstructorAccessibilityCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
- return LangOpts.CPlusPlus;
+ return LangOpts.CPlusPlus11;
}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
index cbbffeafcf90fc..ecf05ab6b3a421 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
@@ -42,7 +42,8 @@ Example:
Bad BadInstance;
To ensure that no accidental instantiation happens, the best practice is to make
-the constructor private and declare the derived class as friend.
+the constructor private and declare the derived class as friend. Note that as a tradeoff,
+this also gives the derived class access to every other private members of the CRTP.
Example:
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index e4efd01007c9dd..fcd21c291eb77c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing
namespace class_implicit_ctor {
template <typename T>
>From 875932e8ca0479af8c9db405d3d5e0bef15c2814 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 01:38:36 +0100
Subject: [PATCH 18/24] format
---
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 0e2957ee21a791..e518a64abc52eb 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -20,6 +20,7 @@
#include "ChainedComparisonCheck.h"
#include "ComparePointerToMemberVirtualFunctionCheck.h"
#include "CopyConstructorInitCheck.h"
+#include "CrtpConstructorAccessibilityCheck.h"
#include "DanglingHandleCheck.h"
#include "DynamicStaticInitializersCheck.h"
#include "EasilySwappableParametersCheck.h"
@@ -83,7 +84,6 @@
#include "UnhandledExceptionAtNewCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
#include "UniquePtrArrayMismatchCheck.h"
-#include "CrtpConstructorAccessibilityCheck.h"
#include "UnsafeFunctionsCheck.h"
#include "UnusedLocalNonTrivialVariableCheck.h"
#include "UnusedRaiiCheck.h"
@@ -238,7 +238,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
- CheckFactories.registerCheck<CrtpConstructorAccessibilityCheck>("bugprone-crtp-constructor-accessibility");
+ CheckFactories.registerCheck<CrtpConstructorAccessibilityCheck>(
+ "bugprone-crtp-constructor-accessibility");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(
>From f54de56d49b8d9b48ff5a1ae3b7400c81749f11f Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 02:32:44 +0100
Subject: [PATCH 19/24] comments
---
.../CrtpConstructorAccessibilityCheck.cpp | 71 ++++++++-----------
.../crtp-constructor-accessibility.cpp | 4 +-
2 files changed, 33 insertions(+), 42 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index 86f291a83b1875..8520d124ad0c16 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -16,69 +16,59 @@ using namespace clang::ast_matchers;
namespace clang::tidy::bugprone {
static bool hasPrivateConstructor(const CXXRecordDecl *RD) {
- for (auto &&Ctor : RD->ctors()) {
- if (Ctor->getAccess() == AS_private)
- return true;
- }
-
- return false;
+ return llvm::any_of(RD->ctors(), [](const CXXConstructorDecl *Ctor) {
+ return Ctor->getAccess() == AS_private;
+ });
}
static bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
const NamedDecl *Param) {
- for (auto &&Friend : CRTP->friends()) {
+ return llvm::any_of(CRTP->friends(), [&](const FriendDecl *Friend) {
const auto *TTPT =
dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
- if (TTPT && TTPT->getDecl() == Param)
- return true;
- }
-
- return false;
+ return TTPT && TTPT->getDecl() == Param;
+ });
}
static bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
const CXXRecordDecl *Derived) {
- for (auto &&Friend : CRTP->friends()) {
- if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
- return true;
- }
-
- return false;
+ return llvm::any_of(CRTP->friends(), [&](const FriendDecl *Friend) {
+ return Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived;
+ });
}
-static std::optional<const NamedDecl *>
+static const NamedDecl *
getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
const CXXRecordDecl *Derived) {
size_t Idx = 0;
- bool Found = false;
- for (auto &&TemplateArg : CRTP->getTemplateArgs().asArray()) {
- if (TemplateArg.getKind() == TemplateArgument::Type &&
- TemplateArg.getAsType()->getAsCXXRecordDecl() == Derived) {
- Found = true;
- break;
- }
- ++Idx;
- }
-
- if (!Found)
- return std::nullopt;
-
- return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
+ bool AnyOf = llvm::any_of(
+ CRTP->getTemplateArgs().asArray(), [&](const TemplateArgument &Arg) {
+ ++Idx;
+ return Arg.getKind() == TemplateArgument::Type &&
+ Arg.getAsType()->getAsCXXRecordDecl() == Derived;
+ });
+
+ return AnyOf ? CRTP->getSpecializedTemplate()
+ ->getTemplateParameters()
+ ->getParam(Idx - 1)
+ : nullptr;
}
static std::vector<FixItHint>
hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
- const std::string &OriginalAccess, const SourceManager &SM,
- const LangOptions &LangOpts) {
+ const std::string &OriginalAccess) {
std::vector<FixItHint> Hints;
Hints.emplace_back(FixItHint::CreateInsertion(
Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
+ const ASTContext &ASTCtx = Ctor->getASTContext();
const SourceLocation CtorEndLoc =
Ctor->isExplicitlyDefaulted()
- ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(),
+ ASTCtx.getSourceManager(),
+ ASTCtx.getLangOpts())
: Ctor->getEndLoc();
Hints.emplace_back(FixItHint::CreateInsertion(
CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
@@ -121,7 +111,10 @@ void CrtpConstructorAccessibilityCheck::check(
}
const auto *DerivedTemplateParameter =
- *getDerivedParameter(CRTPInstantiation, DerivedRecord);
+ getDerivedParameter(CRTPInstantiation, DerivedRecord);
+
+ assert(DerivedTemplateParameter &&
+ "No template parameter corresponds to the derived class of the CRTP.");
if (hasPrivateConstructor(CRTPDeclaration) &&
!isDerivedParameterBefriended(CRTPDeclaration,
@@ -148,9 +141,7 @@ void CrtpConstructorAccessibilityCheck::check(
diag(Ctor->getLocation(),
"%0 contructor allows the CRTP to be %select{inherited "
"from|constructed}1 as a regular template class")
- << Access << IsPublic << Ctor
- << hintMakeCtorPrivate(Ctor, Access, *Result.SourceManager,
- getLangOpts());
+ << Access << IsPublic << Ctor << hintMakeCtorPrivate(Ctor, Access);
diag(Ctor->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index fcd21c291eb77c..df0611c62bd93c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -10,7 +10,7 @@ class CRTP {};
class A : CRTP<A> {};
} // namespace class_implicit_ctor
-namespace class_uncostructible {
+namespace class_unconstructible {
template <typename T>
class CRTP {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-crtp-constructor-accessibility]
@@ -20,7 +20,7 @@ class CRTP {
};
class A : CRTP<A> {};
-} // namespace class_uncostructible
+} // namespace class_unconstructible
namespace class_public_default_ctor {
template <typename T>
>From 22ad6947ca2414509bcae99532cb03d7c834ee2d Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 03:22:56 +0100
Subject: [PATCH 20/24] concatenate friend fix with others
---
.../CrtpConstructorAccessibilityCheck.cpp | 86 ++++++++++++-------
.../crtp-constructor-accessibility.cpp | 44 +++++++---
2 files changed, 86 insertions(+), 44 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index 8520d124ad0c16..506be9c037820c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -42,7 +42,7 @@ static const NamedDecl *
getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
const CXXRecordDecl *Derived) {
size_t Idx = 0;
- bool AnyOf = llvm::any_of(
+ const bool AnyOf = llvm::any_of(
CRTP->getTemplateArgs().asArray(), [&](const TemplateArgument &Arg) {
++Idx;
return Arg.getKind() == TemplateArgument::Type &&
@@ -95,38 +95,52 @@ void CrtpConstructorAccessibilityCheck::check(
const CXXRecordDecl *CRTPDeclaration =
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
- if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
- const bool IsStruct = CRTPDeclaration->isStruct();
-
- diag(CRTPDeclaration->getLocation(),
- "the implicit default constructor of the CRTP is publicly accessible")
- << CRTPDeclaration
- << FixItHint::CreateInsertion(
- CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
- (IsStruct ? "\nprivate:\n" : "\n") +
- CRTPDeclaration->getNameAsString() + "() = default;\n" +
- (IsStruct ? "public:\n" : ""));
- diag(CRTPDeclaration->getLocation(), "consider making it private",
- DiagnosticIDs::Note);
- }
-
const auto *DerivedTemplateParameter =
getDerivedParameter(CRTPInstantiation, DerivedRecord);
assert(DerivedTemplateParameter &&
"No template parameter corresponds to the derived class of the CRTP.");
- if (hasPrivateConstructor(CRTPDeclaration) &&
- !isDerivedParameterBefriended(CRTPDeclaration,
- DerivedTemplateParameter) &&
- !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord)) {
+ bool NeedsFriend = !isDerivedParameterBefriended(CRTPDeclaration,
+ DerivedTemplateParameter) &&
+ !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord);
+
+ const FixItHint HintFriend = FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getEnd(),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';' + '\n');
+
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ const bool IsStruct = CRTPDeclaration->isStruct();
+
+ // FIXME: Clean this up!
+ {
+ DiagnosticBuilder Diag =
+ diag(CRTPDeclaration->getLocation(),
+ "the implicit default constructor of the CRTP is publicly "
+ "accessible")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(
+ 1),
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : ""));
+
+ if (NeedsFriend)
+ Diag << HintFriend;
+ }
+
+ diag(CRTPDeclaration->getLocation(),
+ "consider making it private%select{| and declaring the derived class "
+ "as friend}0",
+ DiagnosticIDs::Note)
+ << NeedsFriend;
+ }
+
+ if (hasPrivateConstructor(CRTPDeclaration) && NeedsFriend) {
diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
- << CRTPDeclaration
- << FixItHint::CreateInsertion(
- CRTPDeclaration->getBraceRange().getEnd(),
- "friend " + DerivedTemplateParameter->getNameAsString() + ';' +
- '\n');
+ << CRTPDeclaration << HintFriend;
diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
@@ -138,12 +152,24 @@ void CrtpConstructorAccessibilityCheck::check(
const bool IsPublic = Ctor->getAccess() == AS_public;
const std::string Access = IsPublic ? "public" : "protected";
+ // FIXME: Clean this up!
+ {
+ DiagnosticBuilder Diag =
+ diag(Ctor->getLocation(),
+ "%0 contructor allows the CRTP to be %select{inherited "
+ "from|constructed}1 as a regular template class")
+ << Access << IsPublic << Ctor << hintMakeCtorPrivate(Ctor, Access);
+
+ if (NeedsFriend)
+ Diag << HintFriend;
+ }
+
+ // FIXME: Test the NeedsFriend false case!
diag(Ctor->getLocation(),
- "%0 contructor allows the CRTP to be %select{inherited "
- "from|constructed}1 as a regular template class")
- << Access << IsPublic << Ctor << hintMakeCtorPrivate(Ctor, Access);
- diag(Ctor->getLocation(), "consider making it private",
- DiagnosticIDs::Note);
+ "consider making it private%select{| and declaring the derived class "
+ "as friend}0",
+ DiagnosticIDs::Note)
+ << NeedsFriend;
}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index df0611c62bd93c..ab8f3a92299656 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -4,8 +4,9 @@ namespace class_implicit_ctor {
template <typename T>
class CRTP {};
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: CRTP() = default;
+// CHECK-FIXES: friend T;
class A : CRTP<A> {};
} // namespace class_implicit_ctor
@@ -28,8 +29,9 @@ class CRTP {
public:
CRTP() = default;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+ // CHECK-FIXES: friend T;
};
class A : CRTP<A> {};
@@ -41,8 +43,9 @@ class CRTP {
public:
CRTP(int) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+ // CHECK-FIXES: friend T;
};
class A : CRTP<A> {};
@@ -54,12 +57,15 @@ class CRTP {
public:
CRTP(int) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
CRTP(float) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}public:
+
+ // CHECK-FIXES: friend T;
+ // CHECK-FIXES: friend T;
};
class A : CRTP<A> {};
@@ -71,16 +77,20 @@ class CRTP {
protected:
CRTP(int) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}protected:
CRTP() = default;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}protected:
CRTP(float) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}protected:
+
+ // CHECK-FIXES: friend T;
+ // CHECK-FIXES: friend T;
+ // CHECK-FIXES: friend T;
};
class A : CRTP<A> {};
@@ -90,8 +100,9 @@ namespace struct_implicit_ctor {
template <typename T>
struct CRTP {};
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+// CHECK-FIXES: friend T;
class A : CRTP<A> {};
} // namespace struct_implicit_ctor
@@ -101,8 +112,9 @@ template <typename T>
struct CRTP {
CRTP() = default;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-crtp-constructor-accessibility]
- // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+ // CHECK-FIXES: friend T;
};
class A : CRTP<A> {};
@@ -112,14 +124,16 @@ namespace same_class_multiple_crtps {
template <typename T>
struct CRTP {};
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+// CHECK-FIXES: friend T;
template <typename T>
struct CRTP2 {};
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: private:{{[[:space:]]*}}CRTP2() = default;{{[[:space:]]*}}public:
+// CHECK-FIXES: friend T;
class A : CRTP<A>, CRTP2<A> {};
} // namespace same_class_multiple_crtps
@@ -165,8 +179,9 @@ namespace template_derived {
template <typename T>
class CRTP {};
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: CRTP() = default;
+// CHECK-FIXES: friend T;
template<typename T>
class A : CRTP<A<T>> {};
@@ -182,8 +197,9 @@ namespace template_derived_explicit_specialization {
template <typename T>
class CRTP {};
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
-// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private and declaring the derived class as friend
// CHECK-FIXES: CRTP() = default;
+// CHECK-FIXES: friend T;
template<typename T>
class A : CRTP<A<T>> {};
>From 63dc68b03e9040171f4e0ea0097c15437a30bb49 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 19:59:30 +0100
Subject: [PATCH 21/24] test
---
.../bugprone/CrtpConstructorAccessibilityCheck.cpp | 1 -
.../bugprone/crtp-constructor-accessibility.cpp | 14 ++++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index 506be9c037820c..ff860bb51117b0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -164,7 +164,6 @@ void CrtpConstructorAccessibilityCheck::check(
Diag << HintFriend;
}
- // FIXME: Test the NeedsFriend false case!
diag(Ctor->getLocation(),
"consider making it private%select{| and declaring the derived class "
"as friend}0",
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index ab8f3a92299656..8b31ed55380e2a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -236,6 +236,20 @@ class A : CRTP<A> {};
class B : CRTP<B> {};
} // namespace explicit_derived_friend_multiple
+namespace no_need_for_friend {
+class A;
+
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-crtp-constructor-accessibility]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+ friend A;
+};
+
+class A : CRTP<A> {};
+} // namespace no_need_for_friend
+
namespace no_warning {
template <typename T>
class CRTP
>From f2d4465420fdd852644b90b1d3a34d380e19899b Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 20:29:50 +0100
Subject: [PATCH 22/24] cleanup
---
.../CrtpConstructorAccessibilityCheck.cpp | 88 +++++++++----------
1 file changed, 43 insertions(+), 45 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index ff860bb51117b0..ffdd53c27ce468 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -109,34 +109,6 @@ void CrtpConstructorAccessibilityCheck::check(
CRTPDeclaration->getBraceRange().getEnd(),
"friend " + DerivedTemplateParameter->getNameAsString() + ';' + '\n');
- if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
- const bool IsStruct = CRTPDeclaration->isStruct();
-
- // FIXME: Clean this up!
- {
- DiagnosticBuilder Diag =
- diag(CRTPDeclaration->getLocation(),
- "the implicit default constructor of the CRTP is publicly "
- "accessible")
- << CRTPDeclaration
- << FixItHint::CreateInsertion(
- CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(
- 1),
- (IsStruct ? "\nprivate:\n" : "\n") +
- CRTPDeclaration->getNameAsString() + "() = default;\n" +
- (IsStruct ? "public:\n" : ""));
-
- if (NeedsFriend)
- Diag << HintFriend;
- }
-
- diag(CRTPDeclaration->getLocation(),
- "consider making it private%select{| and declaring the derived class "
- "as friend}0",
- DiagnosticIDs::Note)
- << NeedsFriend;
- }
-
if (hasPrivateConstructor(CRTPDeclaration) && NeedsFriend) {
diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
@@ -145,6 +117,41 @@ void CrtpConstructorAccessibilityCheck::check(
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
+ auto DiagNoteFriendPrivate = [&](const SourceLocation &Loc, bool Friend) {
+ return diag(Loc,
+ "consider making it private%select{| and declaring the "
+ "derived class "
+ "as friend}0",
+ DiagnosticIDs::Note)
+ << Friend;
+ };
+
+ auto WithFriendHintIfNeeded = [&](DiagnosticBuilder Diag, bool NeedsFriend) {
+ if (NeedsFriend)
+ Diag << HintFriend;
+
+ return Diag;
+ };
+
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ const bool IsStruct = CRTPDeclaration->isStruct();
+
+ WithFriendHintIfNeeded(
+ diag(CRTPDeclaration->getLocation(),
+ "the implicit default constructor of the CRTP is publicly "
+ "accessible")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(
+ 1),
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : "")),
+ NeedsFriend);
+
+ DiagNoteFriendPrivate(CRTPDeclaration->getLocation(), NeedsFriend);
+ }
+
for (auto &&Ctor : CRTPDeclaration->ctors()) {
if (Ctor->getAccess() == AS_private)
continue;
@@ -152,23 +159,14 @@ void CrtpConstructorAccessibilityCheck::check(
const bool IsPublic = Ctor->getAccess() == AS_public;
const std::string Access = IsPublic ? "public" : "protected";
- // FIXME: Clean this up!
- {
- DiagnosticBuilder Diag =
- diag(Ctor->getLocation(),
- "%0 contructor allows the CRTP to be %select{inherited "
- "from|constructed}1 as a regular template class")
- << Access << IsPublic << Ctor << hintMakeCtorPrivate(Ctor, Access);
-
- if (NeedsFriend)
- Diag << HintFriend;
- }
-
- diag(Ctor->getLocation(),
- "consider making it private%select{| and declaring the derived class "
- "as friend}0",
- DiagnosticIDs::Note)
- << NeedsFriend;
+ WithFriendHintIfNeeded(
+ diag(Ctor->getLocation(),
+ "%0 contructor allows the CRTP to be %select{inherited "
+ "from|constructed}1 as a regular template class")
+ << Access << IsPublic << Ctor << hintMakeCtorPrivate(Ctor, Access),
+ NeedsFriend);
+
+ DiagNoteFriendPrivate(Ctor->getLocation(), NeedsFriend);
}
}
>From 44bbb719bdeafb9e850caa81050226d97f3db3ed Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 20:37:49 +0100
Subject: [PATCH 23/24] test
---
.../bugprone/crtp-constructor-accessibility.cpp | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index 8b31ed55380e2a..d2e300389bbe72 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -260,3 +260,16 @@ class CRTP
class A : CRTP<A> {};
} // namespace no_warning
+
+namespace no_warning_unsupported {
+template<typename... Types>
+class CRTP
+{};
+
+class A : CRTP<A> {};
+
+void foo() {
+ A A;
+ (void) A;
+}
+} // namespace no_warning_unsupported
>From 250188a16123e2074052f44f88015c66b25ca9fd Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 23 Feb 2024 21:14:22 +0100
Subject: [PATCH 24/24] docs
---
.../crtp-constructor-accessibility.rst | 21 ++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
index ecf05ab6b3a421..b902b6439a4637 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
@@ -3,7 +3,26 @@
bugprone-crtp-constructor-accessibility
=======================================
-Finds CRTP used in an error-prone way.
+Finds Curiously Recurring Template Pattern used in an error-prone way.
+
+The CRTP is an idiom, in which a class derives from a template class, where
+itself is the template argument. It should be ensured that if a class is
+intended to be a base class in this idiom, it can only be instantiated if
+the derived class is it's template argument.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ private:
+ CRTP() = default;
+ friend T;
+ };
+
+ class Derived : CRTP<Derived> {};
+
+Below can be seen some common mistakes that will allow the breaking of the idiom.
If the constructor of a class intended to be used in a CRTP is public, then
it allows users to construct that class on its own.
More information about the cfe-commits
mailing list