[clang-tools-extra] [clang-tidy] Added check 'bugprone-function-visibility-change' (PR #140086)
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Thu May 29 09:28:42 PDT 2025
https://github.com/balazske updated https://github.com/llvm/llvm-project/pull/140086
>From 65d44a4eb9621e49a96f1ac43e5a1bbd6691dc13 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Thu, 15 May 2025 17:41:16 +0200
Subject: [PATCH 1/5] [clang-tidy] Added check
'bugprone-function-visibility-change'
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../FunctionVisibilityChangeCheck.cpp | 74 ++++++
.../bugprone/FunctionVisibilityChangeCheck.h | 33 +++
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../bugprone/function-visibility-change.rst | 43 ++++
.../docs/clang-tidy/checks/list.rst | 1 +
.../bugprone/function-visibility-change.cpp | 234 ++++++++++++++++++
8 files changed, 394 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index b780a85bdf3fe..7cf58c5218969 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -30,6 +30,7 @@
#include "FoldInitTypeCheck.h"
#include "ForwardDeclarationNamespaceCheck.h"
#include "ForwardingReferenceOverloadCheck.h"
+#include "FunctionVisibilityChangeCheck.h"
#include "ImplicitWideningOfMultiplicationResultCheck.h"
#include "InaccurateEraseCheck.h"
#include "IncDecInConditionsCheck.h"
@@ -143,6 +144,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-forward-declaration-namespace");
CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
"bugprone-forwarding-reference-overload");
+ CheckFactories.registerCheck<FunctionVisibilityChangeCheck>(
+ "bugprone-function-visibility-change");
CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
"bugprone-implicit-widening-of-multiplication-result");
CheckFactories.registerCheck<InaccurateEraseCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index e310ea9c94543..b4f7ba76f4cee 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -26,6 +26,7 @@ add_clang_library(clangTidyBugproneModule STATIC
FoldInitTypeCheck.cpp
ForwardDeclarationNamespaceCheck.cpp
ForwardingReferenceOverloadCheck.cpp
+ FunctionVisibilityChangeCheck.cpp
ImplicitWideningOfMultiplicationResultCheck.cpp
InaccurateEraseCheck.cpp
IncorrectEnableIfCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
new file mode 100644
index 0000000000000..7ea4ed20705ed
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
@@ -0,0 +1,74 @@
+//===--- FunctionVisibilityChangeCheck.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 "FunctionVisibilityChangeCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ cxxMethodDecl(
+ ofClass(cxxRecordDecl().bind("class")),
+ forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base")))
+ .bind("base_func")))
+ .bind("func"),
+ this);
+}
+
+void FunctionVisibilityChangeCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *MatchedFunction = Result.Nodes.getNodeAs<FunctionDecl>("func");
+ const auto *ParentClass = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
+ const auto *OverriddenFunction =
+ Result.Nodes.getNodeAs<FunctionDecl>("base_func");
+ const auto *BaseClass = Result.Nodes.getNodeAs<CXXRecordDecl>("base");
+
+ if (!MatchedFunction->isCanonicalDecl())
+ return;
+
+ AccessSpecifier ActualAccess = MatchedFunction->getAccess();
+ AccessSpecifier OverriddenAccess = OverriddenFunction->getAccess();
+
+ CXXBasePaths Paths;
+ if (!ParentClass->isDerivedFrom(BaseClass, Paths))
+ return;
+ const CXXBaseSpecifier *InheritanceWithStrictVisibility = nullptr;
+ for (const CXXBasePath &Path : Paths) {
+ for (auto Elem : Path) {
+ if (Elem.Base->getAccessSpecifier() > OverriddenAccess) {
+ OverriddenAccess = Elem.Base->getAccessSpecifier();
+ InheritanceWithStrictVisibility = Elem.Base;
+ }
+ }
+ }
+
+ if (ActualAccess != OverriddenAccess) {
+ if (InheritanceWithStrictVisibility) {
+ diag(MatchedFunction->getLocation(),
+ "visibility of function %0 is changed from %1 (through %1 "
+ "inheritance of class %2) to %3")
+ << MatchedFunction << OverriddenAccess
+ << InheritanceWithStrictVisibility->getType() << ActualAccess;
+ diag(InheritanceWithStrictVisibility->getBeginLoc(),
+ "this inheritance would make %0 %1", DiagnosticIDs::Note)
+ << MatchedFunction << OverriddenAccess;
+ } else {
+ diag(MatchedFunction->getLocation(),
+ "visibility of function %0 is changed from %1 in class %2 to %3")
+ << MatchedFunction << OverriddenAccess << BaseClass << ActualAccess;
+ }
+ diag(OverriddenFunction->getLocation(), "function declared here as %0",
+ DiagnosticIDs::Note)
+ << OverriddenFunction->getAccess();
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
new file mode 100644
index 0000000000000..ec7152fb63acb
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
@@ -0,0 +1,33 @@
+//===--- FunctionVisibilityChangeCheck.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_FUNCTIONVISIBILITYCHANGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FUNCTIONVISIBILITYCHANGECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Check function visibility changes in subclasses.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/function-visibility-change.html
+class FunctionVisibilityChangeCheck : public ClangTidyCheck {
+public:
+ FunctionVisibilityChangeCheck(StringRef Name, ClangTidyContext *Context)
+ : 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 {
+ return LangOpts.CPlusPlus;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FUNCTIONVISIBILITYCHANGECHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 579fca54924d5..52f61a78fa3b7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -124,6 +124,11 @@ New checks
pointer and store it as class members without handle the copy and move
constructors and the assignments.
+- New :doc:`bugprone-function-visibility-change
+ <clang-tidy/checks/bugprone/function-visibility-change>` check.
+
+ Check function visibility changes in subclasses.
+
- New :doc:`bugprone-unintended-char-ostream-output
<clang-tidy/checks/bugprone/unintended-char-ostream-output>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
new file mode 100644
index 0000000000000..3b7940e5e584e
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - bugprone-function-visibility-change
+
+bugprone-function-visibility-change
+===================================
+
+Check changes in visibility of C++ member functions in subclasses. The check
+detects if a virtual function is overridden with a different visibility than in
+the base class declaration. Only normal functions are detected, no constructors,
+operators, conversions or other special functions.
+
+.. code-block:: c++
+
+ class A {
+ public:
+ virtual void f_pub();
+ private:
+ virtual void f_priv();
+ };
+
+ class B: public A {
+ public:
+ void f_priv(); // warning: changed visibility from private to public
+ private:
+ void f_pub(); // warning: changed visibility from public to private
+ };
+
+ class C: private A {
+ // no warning: f_pub becomes private in this case but this is from the
+ // private inheritance
+ };
+
+ class D: private A {
+ public:
+ void f_pub(); // warning: changed visibility from private to public
+ // 'f_pub' would have private access but is forced to be
+ // public
+ };
+
+The changed visibility can be an indicator of bad design or a result of
+coding error or code changes. If it is intentional, it can be avoided by
+adding an additional virtual function with the new access.
+
+
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 18f1467285fab..59653a2059e64 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -98,6 +98,7 @@ Clang-Tidy Checks
:doc:`bugprone-fold-init-type <bugprone/fold-init-type>`,
:doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`,
:doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`,
+ :doc:`bugprone-function-visibility-change <bugprone/function-visibility-change>`, "Yes"
:doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes"
:doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
:doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
new file mode 100644
index 0000000000000..eb4532374267b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
@@ -0,0 +1,234 @@
+// RUN: %check_clang_tidy %s bugprone-function-visibility-change %t
+
+class A {
+public:
+ virtual void pub_foo1() {}
+ virtual void pub_foo2() {}
+ virtual void pub_foo3() {}
+protected:
+ virtual void prot_foo1();
+ virtual void prot_foo2();
+ virtual void prot_foo3();
+private:
+ virtual void priv_foo1() {}
+ virtual void priv_foo2() {}
+ virtual void priv_foo3() {}
+};
+
+void A::prot_foo1() {}
+void A::prot_foo2() {}
+void A::prot_foo3() {}
+
+namespace test1 {
+
+class B: public A {
+public:
+ void pub_foo1() override {}
+ void prot_foo1() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'A' to public [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :9:16: note: function declared here as protected
+ void priv_foo1() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'A' to public [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :13:16: note: function declared here as private
+protected:
+ void pub_foo2() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo2' is changed from public in class 'A' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :6:16: note: function declared here as public
+ void prot_foo2() override {}
+ void priv_foo2() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :14:16: note: function declared here as private
+private:
+ void pub_foo3() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo3' is changed from public in class 'A' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :7:16: note: function declared here as public
+ void prot_foo3() override {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo3' is changed from protected in class 'A' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :11:16: note: function declared here as protected
+ void priv_foo3() override {}
+};
+
+class C: public B {
+public:
+ void pub_foo1() override;
+protected:
+ void prot_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from public in class 'B' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :27:8: note: function declared here as public
+private:
+ void priv_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from public in class 'B' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :30:8: note: function declared here as public
+};
+
+void C::prot_foo1() {}
+void C::priv_foo1() {}
+
+}
+
+namespace test2 {
+
+class B: public A {
+public:
+ void pub_foo1() override;
+protected:
+ void prot_foo1() override;
+private:
+ void priv_foo1() override;
+};
+
+class C: public B {
+public:
+ void pub_foo1() override;
+ void prot_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'B' to public
+ // CHECK-MESSAGES: :75:8: note: function declared here as protected
+ void priv_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'B' to public
+ // CHECK-MESSAGES: :77:8: note: function declared here as private
+
+ void pub_foo2() override;
+ void prot_foo2() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo2' is changed from protected in class 'A' to public
+ // CHECK-MESSAGES: :10:16: note: function declared here as protected
+ void priv_foo2() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to public
+ // CHECK-MESSAGES: :14:16: note: function declared here as private
+};
+
+}
+
+namespace test3 {
+
+class B: private A {
+public:
+ void pub_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
+ // CHECK-MESSAGES: :103:10: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :5:16: note: function declared here as public
+protected:
+ void prot_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from private (through private inheritance of class 'A') to protected
+ // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo1' private
+ // CHECK-MESSAGES: :9:16: note: function declared here as protected
+private:
+ void priv_foo1() override;
+
+public:
+ void prot_foo2() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo2' is changed from private (through private inheritance of class 'A') to public
+ // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo2' private
+ // CHECK-MESSAGES: :10:16: note: function declared here as protected
+ void priv_foo2() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to public
+ // CHECK-MESSAGES: :14:16: note: function declared here as private
+
+private:
+ void pub_foo3() override;
+ void prot_foo3() override;
+};
+
+class C: private A {
+};
+
+class D: public C {
+public:
+ void pub_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
+ // CHECK-MESSAGES: :131:10: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :5:16: note: function declared here as public
+};
+
+
+}
+
+namespace test4 {
+
+struct Base1 {
+public:
+ virtual void foo1();
+private:
+ virtual void foo2();
+};
+
+struct Base2 {
+public:
+ virtual void foo2();
+private:
+ virtual void foo1();
+};
+
+struct A : public Base1, public Base2 {
+protected:
+ void foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'foo1' is changed from private in class 'Base2' to protected
+ // CHECK-MESSAGES: :158:16: note: function declared here as private
+ // CHECK-MESSAGES: :[[@LINE-3]]:8: warning: visibility of function 'foo1' is changed from public in class 'Base1' to protected
+ // CHECK-MESSAGES: :149:16: note: function declared here as public
+private:
+ void foo2() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'foo2' is changed from public in class 'Base2' to private
+ // CHECK-MESSAGES: :156:16: note: function declared here as public
+};
+
+}
+
+namespace test5 {
+
+struct B1: virtual public A {};
+struct B2: virtual private A {};
+struct B: public B1, public B2 {
+public:
+ void pub_foo1() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
+ // CHECK-MESSAGES: :179:12: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :5:16: note: function declared here as public
+};
+
+}
+
+namespace test6 {
+class A {
+private:
+ A(int);
+};
+class B: public A {
+public:
+ using A::A;
+};
+}
+
+namespace test7 {
+
+template <typename T>
+class A {
+protected:
+ virtual T foo();
+};
+
+template <typename T>
+class B: public A<T> {
+private:
+ T foo() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from protected in class 'A<int>' to private
+ // CHECK-MESSAGES: :206:13: note: function declared here as protected
+};
+
+template <typename T>
+class C: private A<T> {
+public:
+ T foo() override;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from private (through private inheritance of class 'A<int>') to public
+ // CHECK-MESSAGES: :218:10: note: this inheritance would make 'foo' private
+ // CHECK-MESSAGES: :206:13: note: function declared here as protected
+};
+
+B<int> fB() {
+ return B<int>{};
+}
+
+C<int> fC() {
+ return C<int>{};
+}
+
+}
>From f19ab2e4cd0eee1b261767486cf7826f8c7b7ff6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Mon, 19 May 2025 18:11:40 +0200
Subject: [PATCH 2/5] code and doc fixes, added option
---
.../FunctionVisibilityChangeCheck.cpp | 60 +++++++++++---
.../bugprone/FunctionVisibilityChangeCheck.h | 11 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../bugprone/function-visibility-change.rst | 20 ++---
.../docs/clang-tidy/checks/list.rst | 2 +-
.../test-system-header.h | 14 ++++
.../bugprone/function-visibility-change.cpp | 79 ++++++++++++++++---
7 files changed, 153 insertions(+), 35 deletions(-)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
index 7ea4ed20705ed..f5cfd0c37d4c4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
@@ -11,12 +11,45 @@
using namespace clang::ast_matchers;
-namespace clang::tidy::bugprone {
+namespace clang::tidy {
+
+template <>
+struct OptionEnumMapping<bugprone::FunctionVisibilityChangeCheck::ChangeKind> {
+ static llvm::ArrayRef<
+ std::pair<bugprone::FunctionVisibilityChangeCheck::ChangeKind, StringRef>>
+ getEnumMapping() {
+ static constexpr std::pair<
+ bugprone::FunctionVisibilityChangeCheck::ChangeKind, StringRef>
+ Mapping[] = {
+ {bugprone::FunctionVisibilityChangeCheck::ChangeKind::Any, "any"},
+ {bugprone::FunctionVisibilityChangeCheck::ChangeKind::Widening,
+ "widening"},
+ {bugprone::FunctionVisibilityChangeCheck::ChangeKind::Narrowing,
+ "narrowing"},
+ };
+ return {Mapping};
+ }
+};
+
+namespace bugprone {
+
+FunctionVisibilityChangeCheck::FunctionVisibilityChangeCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ DetectVisibilityChange(
+ Options.get("DisallowedVisibilityChange", ChangeKind::Any)) {}
+
+void FunctionVisibilityChangeCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "DisallowedVisibilityChange", DetectVisibilityChange);
+}
void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
cxxMethodDecl(
- ofClass(cxxRecordDecl().bind("class")),
+ isVirtual(),
+ ofClass(
+ cxxRecordDecl(unless(isExpansionInSystemHeader())).bind("class")),
forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base")))
.bind("base_func")))
.bind("func"),
@@ -26,14 +59,14 @@ void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) {
void FunctionVisibilityChangeCheck::check(
const MatchFinder::MatchResult &Result) {
const auto *MatchedFunction = Result.Nodes.getNodeAs<FunctionDecl>("func");
+ if (!MatchedFunction->isCanonicalDecl())
+ return;
+
const auto *ParentClass = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
const auto *OverriddenFunction =
Result.Nodes.getNodeAs<FunctionDecl>("base_func");
const auto *BaseClass = Result.Nodes.getNodeAs<CXXRecordDecl>("base");
- if (!MatchedFunction->isCanonicalDecl())
- return;
-
AccessSpecifier ActualAccess = MatchedFunction->getAccess();
AccessSpecifier OverriddenAccess = OverriddenFunction->getAccess();
@@ -42,7 +75,7 @@ void FunctionVisibilityChangeCheck::check(
return;
const CXXBaseSpecifier *InheritanceWithStrictVisibility = nullptr;
for (const CXXBasePath &Path : Paths) {
- for (auto Elem : Path) {
+ for (const CXXBasePathElement &Elem : Path) {
if (Elem.Base->getAccessSpecifier() > OverriddenAccess) {
OverriddenAccess = Elem.Base->getAccessSpecifier();
InheritanceWithStrictVisibility = Elem.Base;
@@ -51,6 +84,13 @@ void FunctionVisibilityChangeCheck::check(
}
if (ActualAccess != OverriddenAccess) {
+ if (DetectVisibilityChange == ChangeKind::Widening &&
+ ActualAccess > OverriddenAccess)
+ return;
+ if (DetectVisibilityChange == ChangeKind::Narrowing &&
+ ActualAccess < OverriddenAccess)
+ return;
+
if (InheritanceWithStrictVisibility) {
diag(MatchedFunction->getLocation(),
"visibility of function %0 is changed from %1 (through %1 "
@@ -58,8 +98,8 @@ void FunctionVisibilityChangeCheck::check(
<< MatchedFunction << OverriddenAccess
<< InheritanceWithStrictVisibility->getType() << ActualAccess;
diag(InheritanceWithStrictVisibility->getBeginLoc(),
- "this inheritance would make %0 %1", DiagnosticIDs::Note)
- << MatchedFunction << OverriddenAccess;
+ "%0 is inherited as %1 here", DiagnosticIDs::Note)
+ << InheritanceWithStrictVisibility->getType() << OverriddenAccess;
} else {
diag(MatchedFunction->getLocation(),
"visibility of function %0 is changed from %1 in class %2 to %3")
@@ -71,4 +111,6 @@ void FunctionVisibilityChangeCheck::check(
}
}
-} // namespace clang::tidy::bugprone
+} // namespace bugprone
+
+} // namespace clang::tidy
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
index ec7152fb63acb..9d4b701d51314 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
@@ -13,19 +13,24 @@
namespace clang::tidy::bugprone {
-/// Check function visibility changes in subclasses.
+/// Checks function visibility changes in subclasses.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/function-visibility-change.html
class FunctionVisibilityChangeCheck : public ClangTidyCheck {
public:
- FunctionVisibilityChangeCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ enum class ChangeKind { Any, Widening, Narrowing };
+
+ FunctionVisibilityChangeCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
+
+private:
+ ChangeKind DetectVisibilityChange;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 52f61a78fa3b7..6b752359c712d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -127,7 +127,7 @@ New checks
- New :doc:`bugprone-function-visibility-change
<clang-tidy/checks/bugprone/function-visibility-change>` check.
- Check function visibility changes in subclasses.
+ Checks function visibility changes in subclasses.
- New :doc:`bugprone-unintended-char-ostream-output
<clang-tidy/checks/bugprone/unintended-char-ostream-output>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
index 3b7940e5e584e..4e0744eede128 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
@@ -3,10 +3,13 @@
bugprone-function-visibility-change
===================================
-Check changes in visibility of C++ member functions in subclasses. The check
-detects if a virtual function is overridden with a different visibility than in
-the base class declaration. Only normal functions are detected, no constructors,
-operators, conversions or other special functions.
+Checks changes in visibility of C++ member functions in subclasses. This
+includes for example if a virtual function declared as `private` is overridden
+and declared as `public` in a subclass. The detected change is the modification
+of visibility resulting from keywords `public`, `protected`, `private` at
+overridden virtual functions. Use of the `using` keyword is not considered by
+this check. The check applies to any normal virtual function and optionally to
+destructors or operators.
.. code-block:: c++
@@ -36,8 +39,7 @@ operators, conversions or other special functions.
// public
};
-The changed visibility can be an indicator of bad design or a result of
-coding error or code changes. If it is intentional, it can be avoided by
-adding an additional virtual function with the new access.
-
-
+If the visibility is changed in this way, it can indicate bad design or
+programming error. If the change is necessary, it can be achieved by the
+``using`` keyword in a more safe way (this has no effect on the visibility
+in further subclasses).
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 59653a2059e64..c5b8a73f3af65 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -98,7 +98,7 @@ Clang-Tidy Checks
:doc:`bugprone-fold-init-type <bugprone/fold-init-type>`,
:doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`,
:doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`,
- :doc:`bugprone-function-visibility-change <bugprone/function-visibility-change>`, "Yes"
+ :doc:`bugprone-function-visibility-change <bugprone/function-visibility-change>`,
:doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes"
:doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
:doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h
new file mode 100644
index 0000000000000..e64e1924a1708
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h
@@ -0,0 +1,14 @@
+#pragma clang system_header
+
+namespace sys {
+
+struct Base {
+ virtual void publicF();
+};
+
+struct Derived: public Base {
+private:
+ void publicF() override;
+};
+
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
index eb4532374267b..b2279d45fe47e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy %s bugprone-function-visibility-change %t
-
+// RUN: %check_clang_tidy %s bugprone-function-visibility-change %t -- -- -I %S/Inputs/function-visibility-change
+#include <test-system-header.h>
class A {
public:
virtual void pub_foo1() {}
@@ -104,12 +104,12 @@ class B: private A {
public:
void pub_foo1() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
- // CHECK-MESSAGES: :103:10: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :103:10: note: 'A' is inherited as private here
// CHECK-MESSAGES: :5:16: note: function declared here as public
protected:
void prot_foo1() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from private (through private inheritance of class 'A') to protected
- // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo1' private
+ // CHECK-MESSAGES: :103:10: note: 'A' is inherited as private here
// CHECK-MESSAGES: :9:16: note: function declared here as protected
private:
void priv_foo1() override;
@@ -117,7 +117,7 @@ class B: private A {
public:
void prot_foo2() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo2' is changed from private (through private inheritance of class 'A') to public
- // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo2' private
+ // CHECK-MESSAGES: :103:10: note: 'A' is inherited as private here
// CHECK-MESSAGES: :10:16: note: function declared here as protected
void priv_foo2() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to public
@@ -135,7 +135,7 @@ class D: public C {
public:
void pub_foo1() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
- // CHECK-MESSAGES: :131:10: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :131:10: note: 'A' is inherited as private here
// CHECK-MESSAGES: :5:16: note: function declared here as public
};
@@ -181,24 +181,30 @@ struct B: public B1, public B2 {
public:
void pub_foo1() override;
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public
- // CHECK-MESSAGES: :179:12: note: this inheritance would make 'pub_foo1' private
+ // CHECK-MESSAGES: :179:12: note: 'A' is inherited as private here
// CHECK-MESSAGES: :5:16: note: function declared here as public
};
}
-namespace test6 {
+namespace test_using {
+
class A {
private:
A(int);
+protected:
+ virtual void f();
};
+
class B: public A {
public:
using A::A;
+ using A::f;
};
+
}
-namespace test7 {
+namespace test_template {
template <typename T>
class A {
@@ -211,7 +217,7 @@ class B: public A<T> {
private:
T foo() override;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from protected in class 'A<int>' to private
- // CHECK-MESSAGES: :206:13: note: function declared here as protected
+ // CHECK-MESSAGES: :[[@LINE-8]]:13: note: function declared here as protected
};
template <typename T>
@@ -219,8 +225,8 @@ class C: private A<T> {
public:
T foo() override;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from private (through private inheritance of class 'A<int>') to public
- // CHECK-MESSAGES: :218:10: note: this inheritance would make 'foo' private
- // CHECK-MESSAGES: :206:13: note: function declared here as protected
+ // CHECK-MESSAGES: :[[@LINE-4]]:10: note: 'A<int>' is inherited as private here
+ // CHECK-MESSAGES: :[[@LINE-17]]:13: note: function declared here as protected
};
B<int> fB() {
@@ -232,3 +238,52 @@ C<int> fC() {
}
}
+
+namespace test_system_header {
+
+struct SysDerived: public sys::Base {
+private:
+ void publicF();
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'publicF' is changed from public in class 'Base' to private
+};
+
+}
+
+namespace test_destructor {
+
+class A {
+public:
+ virtual ~A();
+};
+
+class B: public A {
+protected:
+ ~B();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: visibility of function '~B'
+ // CHECK-MESSAGES: :[[@LINE-7]]:11: note: function declared here
+};
+
+}
+
+namespace test_operator {
+
+class A {
+ virtual int operator()(int);
+ virtual A& operator++();
+ virtual operator double() const;
+};
+
+class B: public A {
+protected:
+ int operator()(int);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: visibility of function 'operator()'
+ // CHECK-MESSAGES: :[[@LINE-9]]:15: note: function declared here
+ A& operator++();
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: visibility of function 'operator++'
+ // CHECK-MESSAGES: :[[@LINE-11]]:14: note: function declared here
+ operator double() const;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: visibility of function 'operator double'
+ // CHECK-MESSAGES: :[[@LINE-13]]:11: note: function declared here
+};
+
+}
\ No newline at end of file
>From 8f1b20eb1531d21d65e74e42965b37acea1a15c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Wed, 21 May 2025 12:55:47 +0200
Subject: [PATCH 3/5] improvements, added options, check rename
---
.../bugprone/BugproneTidyModule.cpp | 4 +-
.../FunctionVisibilityChangeCheck.cpp | 24 +++++-
.../bugprone/FunctionVisibilityChangeCheck.h | 5 +-
.../bugprone/function-visibility-change.rst | 45 ----------
.../visibility-change-to-virtual-function.rst | 86 +++++++++++++++++++
.../docs/clang-tidy/checks/list.rst | 2 +-
.../test-system-header.h | 0
...lity-change-to-virtual-function-ignore.cpp | 60 +++++++++++++
...ity-change-to-virtual-function-options.cpp | 75 ++++++++++++++++
...visibility-change-to-virtual-function.cpp} | 20 ++---
10 files changed, 260 insertions(+), 61 deletions(-)
delete mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
rename clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/{function-visibility-change => visibility-change-to-virtual-function}/test-system-header.h (100%)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-ignore.cpp
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-options.cpp
rename clang-tools-extra/test/clang-tidy/checkers/bugprone/{function-visibility-change.cpp => visibility-change-to-virtual-function.cpp} (92%)
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 7cf58c5218969..bef6b876f6960 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -144,8 +144,6 @@ class BugproneModule : public ClangTidyModule {
"bugprone-forward-declaration-namespace");
CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
"bugprone-forwarding-reference-overload");
- CheckFactories.registerCheck<FunctionVisibilityChangeCheck>(
- "bugprone-function-visibility-change");
CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
"bugprone-implicit-widening-of-multiplication-result");
CheckFactories.registerCheck<InaccurateEraseCheck>(
@@ -280,6 +278,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<UseAfterMoveCheck>("bugprone-use-after-move");
CheckFactories.registerCheck<VirtualNearMissCheck>(
"bugprone-virtual-near-miss");
+ CheckFactories.registerCheck<FunctionVisibilityChangeCheck>(
+ "bugprone-visibility-change-to-virtual-function");
}
};
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
index f5cfd0c37d4c4..5ba2f51eb3dd9 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
@@ -7,6 +7,8 @@
//===----------------------------------------------------------------------===//
#include "FunctionVisibilityChangeCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
using namespace clang::ast_matchers;
@@ -37,20 +39,31 @@ FunctionVisibilityChangeCheck::FunctionVisibilityChangeCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
DetectVisibilityChange(
- Options.get("DisallowedVisibilityChange", ChangeKind::Any)) {}
+ Options.get("DisallowedVisibilityChange", ChangeKind::Any)),
+ CheckDestructors(Options.get("CheckDestructors", false)),
+ CheckOperators(Options.get("CheckOperators", false)),
+ IgnoredFunctions(utils::options::parseStringList(
+ Options.get("IgnoredFunctions", ""))) {}
void FunctionVisibilityChangeCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "DisallowedVisibilityChange", DetectVisibilityChange);
+ Options.store(Opts, "CheckDestructors", CheckDestructors);
+ Options.store(Opts, "CheckOperators", CheckOperators);
+ Options.store(Opts, "IgnoredFunctions",
+ utils::options::serializeStringList(IgnoredFunctions));
}
void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) {
+ auto IgnoredDecl =
+ namedDecl(matchers::matchesAnyListedName(IgnoredFunctions));
Finder->addMatcher(
cxxMethodDecl(
isVirtual(),
ofClass(
cxxRecordDecl(unless(isExpansionInSystemHeader())).bind("class")),
- forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base")))
+ forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base")),
+ unless(IgnoredDecl))
.bind("base_func")))
.bind("func"),
this);
@@ -61,6 +74,13 @@ void FunctionVisibilityChangeCheck::check(
const auto *MatchedFunction = Result.Nodes.getNodeAs<FunctionDecl>("func");
if (!MatchedFunction->isCanonicalDecl())
return;
+ DeclarationName::NameKind NK = MatchedFunction->getDeclName().getNameKind();
+ if (!CheckDestructors && NK == DeclarationName::CXXDestructorName)
+ return;
+ if (!CheckOperators && NK != DeclarationName::Identifier &&
+ NK != DeclarationName::CXXConstructorName &&
+ NK != DeclarationName::CXXDestructorName)
+ return;
const auto *ParentClass = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
const auto *OverriddenFunction =
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
index 9d4b701d51314..2c397217faddd 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
@@ -16,7 +16,7 @@ namespace clang::tidy::bugprone {
/// Checks function visibility changes in subclasses.
///
/// For the user-facing documentation see:
-/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/function-visibility-change.html
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.html
class FunctionVisibilityChangeCheck : public ClangTidyCheck {
public:
enum class ChangeKind { Any, Widening, Narrowing };
@@ -31,6 +31,9 @@ class FunctionVisibilityChangeCheck : public ClangTidyCheck {
private:
ChangeKind DetectVisibilityChange;
+ bool CheckDestructors;
+ bool CheckOperators;
+ std::vector<llvm::StringRef> IgnoredFunctions;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
deleted file mode 100644
index 4e0744eede128..0000000000000
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
+++ /dev/null
@@ -1,45 +0,0 @@
-.. title:: clang-tidy - bugprone-function-visibility-change
-
-bugprone-function-visibility-change
-===================================
-
-Checks changes in visibility of C++ member functions in subclasses. This
-includes for example if a virtual function declared as `private` is overridden
-and declared as `public` in a subclass. The detected change is the modification
-of visibility resulting from keywords `public`, `protected`, `private` at
-overridden virtual functions. Use of the `using` keyword is not considered by
-this check. The check applies to any normal virtual function and optionally to
-destructors or operators.
-
-.. code-block:: c++
-
- class A {
- public:
- virtual void f_pub();
- private:
- virtual void f_priv();
- };
-
- class B: public A {
- public:
- void f_priv(); // warning: changed visibility from private to public
- private:
- void f_pub(); // warning: changed visibility from public to private
- };
-
- class C: private A {
- // no warning: f_pub becomes private in this case but this is from the
- // private inheritance
- };
-
- class D: private A {
- public:
- void f_pub(); // warning: changed visibility from private to public
- // 'f_pub' would have private access but is forced to be
- // public
- };
-
-If the visibility is changed in this way, it can indicate bad design or
-programming error. If the change is necessary, it can be achieved by the
-``using`` keyword in a more safe way (this has no effect on the visibility
-in further subclasses).
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
new file mode 100644
index 0000000000000..afd215d199f4a
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
@@ -0,0 +1,86 @@
+.. title:: clang-tidy - bugprone-visibility-change-to-virtual-function
+
+bugprone-visibility-change-to-virtual-function
+==============================================
+
+Checks changes in visibility of C++ member functions in subclasses. This
+includes for example if a virtual function declared as ``private`` is overridden
+and declared as ``public`` in a subclass. The detected change is the
+modification of visibility resulting from keywords ``public``, ``protected``,
+``private`` at overridden virtual functions. Use of the ``using`` keyword is not
+considered by this check. The check applies to any normal virtual function and
+optionally to destructors or operators.
+
+.. code-block:: c++
+
+ class A {
+ public:
+ virtual void f_pub();
+ private:
+ virtual void f_priv();
+ };
+
+ class B: public A {
+ public:
+ void f_priv(); // warning: changed visibility from private to public
+ private:
+ void f_pub(); // warning: changed visibility from public to private
+ };
+
+ class C: private A {
+ // no warning: f_pub becomes private in this case but this is from the
+ // private inheritance
+ };
+
+ class D: private A {
+ public:
+ void f_pub(); // warning: changed visibility from private to public
+ // 'f_pub' would have private access but is forced to be
+ // public
+ };
+
+If the visibility is changed in this way, it can indicate bad design or
+programming error.
+
+If a virtual function is private in a subclass but public in the base class, it
+can still be accessed from a pointer to the subclass if the pointer is converted
+to the base type. Probably private inheritance can be used instead.
+
+A protected virtual function that is made public in a subclass may have valid
+use cases but similar (not exactly same) effect can be achieved with the
+``using`` keyword.
+
+Options
+-------
+
+.. option:: DisallowedVisibilityChange
+
+ Controls what kind of change to the visibility will be detected by the check.
+ Possible values are `any`, `widening`, `narrowing`. For example the
+ `widening` option will produce warning only if the visibility is changed
+ from more restrictive (``private``) to less restrictive (``public``).
+ Default value is `any`.
+
+.. option:: CheckDestructors
+
+ If turned on, the check does apply to destructors too. Otherwise destructors
+ are ignored by the check.
+ Default value is false.
+
+.. option:: CheckOperators
+
+ If turned on, the check does apply to overloaded C++ operators (as virtual
+ member functions) too. This includes other special member functions (like
+ conversions) too. This option is probably useful only in rare cases because
+ operators and conversions are not often virtual functions.
+ Default value is false.
+
+.. option:: IgnoredFunctions
+
+ This option can be used to ignore the check at specific functions.
+ To configure this option, a semicolon-separated list of function names
+ should be provided. The list can contain regular expressions, in this way it
+ is possible to select all functions of a specific class (like `MyClass::.*`)
+ or a specific function of any class (like `my_function` or
+ `::.*::my_function`). The function names are matched at the base class.
+ Default value: empty string.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index c5b8a73f3af65..2b39b0b7f5934 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -98,7 +98,6 @@ Clang-Tidy Checks
:doc:`bugprone-fold-init-type <bugprone/fold-init-type>`,
:doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`,
:doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`,
- :doc:`bugprone-function-visibility-change <bugprone/function-visibility-change>`,
:doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes"
:doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
:doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
@@ -168,6 +167,7 @@ Clang-Tidy Checks
:doc:`bugprone-unused-return-value <bugprone/unused-return-value>`,
:doc:`bugprone-use-after-move <bugprone/use-after-move>`,
:doc:`bugprone-virtual-near-miss <bugprone/virtual-near-miss>`, "Yes"
+ :doc:`bugprone-visibility-change-to-virtual-function <bugprone/visibility-change-to-virtual-function>`,
:doc:`cert-dcl50-cpp <cert/dcl50-cpp>`,
:doc:`cert-dcl58-cpp <cert/dcl58-cpp>`,
:doc:`cert-env33-c <cert/env33-c>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/visibility-change-to-virtual-function/test-system-header.h
similarity index 100%
rename from clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/function-visibility-change/test-system-header.h
rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/visibility-change-to-virtual-function/test-system-header.h
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-ignore.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-ignore.cpp
new file mode 100644
index 0000000000000..324fae5382219
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-ignore.cpp
@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s bugprone-visibility-change-to-virtual-function %t -- \
+// RUN: -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.IgnoredFunctions: 'IgnoreAlways::.*;::a::IgnoreSelected::.*;IgnoreFunctions::f1;ignored_f'}}"
+
+class IgnoreAlways {
+ virtual void f();
+};
+
+class IgnoreSelected {
+ virtual void f();
+};
+
+namespace a {
+class IgnoreAlways {
+ virtual void f();
+};
+class IgnoreSelected {
+ virtual void f();
+};
+}
+
+namespace ignore_always {
+class Test1: public IgnoreAlways {
+public:
+ void f();
+ void ignored_f(int);
+};
+class Test2: public a::IgnoreAlways {
+public:
+ void f();
+};
+}
+
+namespace ignore_selected {
+class Test1: public IgnoreSelected {
+public:
+ void f();
+ // CHECK-NOTES: :[[@LINE-1]]:8: warning: visibility of function 'f'
+ // CHECK-NOTES: :9:16: note: function declared here
+ void ignored_f(int);
+};
+class Test2: public a::IgnoreSelected {
+public:
+ void f();
+};
+}
+
+class IgnoreFunctions {
+ virtual void f1();
+ virtual void f2();
+ virtual void ignored_f();
+};
+
+class IgnoreFunctionsTest: public IgnoreFunctions {
+public:
+ void f1();
+ void f2();
+ // CHECK-NOTES: :[[@LINE-1]]:8: warning: visibility of function 'f2'
+ // CHECK-NOTES: :[[@LINE-9]]:16: note: function declared here
+ void ignored_f();
+};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-options.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-options.cpp
new file mode 100644
index 0000000000000..a74476849d1fa
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function-options.cpp
@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy -check-suffixes=DTORS,WIDENING,NARROWING %s bugprone-visibility-change-to-virtual-function %t -- \
+// RUN: -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.CheckDestructors: true}}"
+
+// RUN: %check_clang_tidy -check-suffixes=OPS,WIDENING,NARROWING %s bugprone-visibility-change-to-virtual-function %t -- \
+// RUN: -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.CheckOperators: true}}"
+
+// RUN: %check_clang_tidy -check-suffixes=WIDENING %s bugprone-visibility-change-to-virtual-function %t -- \
+// RUN: -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.DisallowedVisibilityChange: 'widening'}}"
+
+// RUN: %check_clang_tidy -check-suffixes=NARROWING %s bugprone-visibility-change-to-virtual-function %t -- \
+// RUN: -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.DisallowedVisibilityChange: 'narrowing'}}"
+
+namespace test_change {
+
+class A {
+protected:
+ virtual void f1();
+ virtual void f2();
+};
+
+class B: public A {
+public:
+ void f1();
+ // CHECK-NOTES-WIDENING: :[[@LINE-1]]:8: warning: visibility of function 'f1'
+ // CHECK-NOTES-WIDENING: :[[@LINE-8]]:16: note: function declared here
+private:
+ void f2();
+ // CHECK-NOTES-NARROWING: :[[@LINE-1]]:8: warning: visibility of function 'f2'
+ // CHECK-NOTES-NARROWING: :[[@LINE-11]]:16: note: function declared here
+};
+
+}
+
+namespace test_destructor {
+
+class A {
+public:
+ virtual ~A();
+};
+
+class B: public A {
+protected:
+ ~B();
+ // CHECK-NOTES-DTORS: :[[@LINE-1]]:3: warning: visibility of function '~B'
+ // CHECK-NOTES-DTORS: :[[@LINE-7]]:11: note: function declared here
+};
+
+}
+
+namespace test_operator {
+
+class A {
+ virtual A& operator=(const A&);
+ virtual A& operator++();
+ virtual int operator()(int);
+ virtual operator double() const;
+};
+
+class B: public A {
+protected:
+ A& operator=(const A&);
+ // CHECK-NOTES-OPS: :[[@LINE-1]]:6: warning: visibility of function 'operator='
+ // CHECK-NOTES-OPS: :[[@LINE-10]]:14: note: function declared here
+ A& operator++();
+ // CHECK-NOTES-OPS: :[[@LINE-1]]:6: warning: visibility of function 'operator++'
+ // CHECK-NOTES-OPS: :[[@LINE-12]]:14: note: function declared here
+ int operator()(int);
+ // CHECK-NOTES-OPS: :[[@LINE-1]]:7: warning: visibility of function 'operator()'
+ // CHECK-NOTES-OPS: :[[@LINE-14]]:15: note: function declared here
+ operator double() const;
+ // CHECK-NOTES-OPS: :[[@LINE-1]]:3: warning: visibility of function 'operator double'
+ // CHECK-NOTES-OPS: :[[@LINE-16]]:11: note: function declared here
+};
+
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function.cpp
similarity index 92%
rename from clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function.cpp
index b2279d45fe47e..ce31ea7c56239 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/visibility-change-to-virtual-function.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-function-visibility-change %t -- -- -I %S/Inputs/function-visibility-change
+// RUN: %check_clang_tidy %s bugprone-visibility-change-to-virtual-function %t -- -config="{CheckOptions: {bugprone-visibility-change-to-virtual-function.CheckDestructors: true,bugprone-visibility-change-to-virtual-function.CheckOperators: true}}" -- -I %S/Inputs/visibility-change-to-virtual-function
#include <test-system-header.h>
class A {
public:
@@ -25,25 +25,25 @@ class B: public A {
public:
void pub_foo1() override {}
void prot_foo1() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'A' to public [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'A' to public [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :9:16: note: function declared here as protected
void priv_foo1() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'A' to public [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'A' to public [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :13:16: note: function declared here as private
protected:
void pub_foo2() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo2' is changed from public in class 'A' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo2' is changed from public in class 'A' to protected [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :6:16: note: function declared here as public
void prot_foo2() override {}
void priv_foo2() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to protected [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :14:16: note: function declared here as private
private:
void pub_foo3() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo3' is changed from public in class 'A' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo3' is changed from public in class 'A' to private [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :7:16: note: function declared here as public
void prot_foo3() override {}
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo3' is changed from protected in class 'A' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo3' is changed from protected in class 'A' to private [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :11:16: note: function declared here as protected
void priv_foo3() override {}
};
@@ -53,11 +53,11 @@ class C: public B {
void pub_foo1() override;
protected:
void prot_foo1() override;
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from public in class 'B' to protected [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from public in class 'B' to protected [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :27:8: note: function declared here as public
private:
void priv_foo1() override;
- // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from public in class 'B' to private [bugprone-function-visibility-change]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from public in class 'B' to private [bugprone-visibility-change-to-virtual-function]
// CHECK-MESSAGES: :30:8: note: function declared here as public
};
@@ -286,4 +286,4 @@ class B: public A {
// CHECK-MESSAGES: :[[@LINE-13]]:11: note: function declared here
};
-}
\ No newline at end of file
+}
>From a63ba1e73aaecfc452878c3efa159e2ddbbba905 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Thu, 22 May 2025 11:57:49 +0200
Subject: [PATCH 4/5] fixed release notes
---
clang-tools-extra/docs/ReleaseNotes.rst | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b752359c712d..2e0767a5c7d68 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -124,17 +124,17 @@ New checks
pointer and store it as class members without handle the copy and move
constructors and the assignments.
-- New :doc:`bugprone-function-visibility-change
- <clang-tidy/checks/bugprone/function-visibility-change>` check.
-
- Checks function visibility changes in subclasses.
-
- New :doc:`bugprone-unintended-char-ostream-output
<clang-tidy/checks/bugprone/unintended-char-ostream-output>` check.
Finds unintended character output from ``unsigned char`` and ``signed char``
to an ``ostream``.
+- New :doc:`bugprone-visibility-change-to-virtual-functions
+ <clang-tidy/checks/bugprone/visibility-change-to-virtual-functions>` check.
+
+ Checks function visibility changes in subclasses.
+
- New :doc:`readability-ambiguous-smartptr-reset-call
<clang-tidy/checks/readability/ambiguous-smartptr-reset-call>` check.
>From db799720492809b614dc643c0812bbb36f76f55e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Thu, 29 May 2025 18:28:08 +0200
Subject: [PATCH 5/5] improved documentation
---
.../bugprone/FunctionVisibilityChangeCheck.h | 3 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 3 ++-
.../visibility-change-to-virtual-function.rst | 19 ++++++++++---------
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
index 2c397217faddd..edb30e736e5fa 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
@@ -13,7 +13,8 @@
namespace clang::tidy::bugprone {
-/// Checks function visibility changes in subclasses.
+/// Finds virtual function overrides with different visibility than the function
+/// in the base class.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.html
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2e0767a5c7d68..66fea340c9d08 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,7 +133,8 @@ New checks
- New :doc:`bugprone-visibility-change-to-virtual-functions
<clang-tidy/checks/bugprone/visibility-change-to-virtual-functions>` check.
- Checks function visibility changes in subclasses.
+ Finds virtual function overrides with different visibility than the function
+ in the base class.
- New :doc:`readability-ambiguous-smartptr-reset-call
<clang-tidy/checks/readability/ambiguous-smartptr-reset-call>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
index afd215d199f4a..11ef81fe80b8e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
@@ -3,13 +3,14 @@
bugprone-visibility-change-to-virtual-function
==============================================
-Checks changes in visibility of C++ member functions in subclasses. This
-includes for example if a virtual function declared as ``private`` is overridden
-and declared as ``public`` in a subclass. The detected change is the
-modification of visibility resulting from keywords ``public``, ``protected``,
-``private`` at overridden virtual functions. Use of the ``using`` keyword is not
-considered by this check. The check applies to any normal virtual function and
-optionally to destructors or operators.
+Finds virtual function overrides with different visibility than the function
+in the base class. This includes for example if a virtual function declared as
+``private`` is overridden and declared as ``public`` in a subclass. The detected
+change is the modification of visibility resulting from keywords ``public``,
+``protected``, ``private`` at overridden virtual functions. The check applies to
+any normal virtual function and optionally to destructors or operators. Use of
+the ``using`` keyword is not considered as visibility change by this check.
+
.. code-block:: c++
@@ -65,7 +66,7 @@ Options
If turned on, the check does apply to destructors too. Otherwise destructors
are ignored by the check.
- Default value is false.
+ Default value is `false`.
.. option:: CheckOperators
@@ -73,7 +74,7 @@ Options
member functions) too. This includes other special member functions (like
conversions) too. This option is probably useful only in rare cases because
operators and conversions are not often virtual functions.
- Default value is false.
+ Default value is `false`.
.. option:: IgnoredFunctions
More information about the cfe-commits
mailing list