[clang-tidy] Added check 'misc-override-with-different-visibility' #140086
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Balázs Kéri (balazske) ChangesFull diff: https://github.com/llvm/llvm-project/pull/140086.diff 8 Files Affected:
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>{};
+}
+
+}
|
clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
Outdated
Show resolved
Hide resolved
NagyDonat
approved these changes
May 17, 2025
Contributor
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
LGTM if you implement the changes suggested by EugeneZelenko.
PiotrZSL
requested changes
May 17, 2025
Member
PiotrZSL
left a comment
PiotrZSL
left a comment
There was a problem hiding this comment.
Idea is ok.
Missing some tests (scenarios not covered).
Maybe some options to deal with big number of false-positives would be needed.
Maybe some option to allow changing visibility to less visible would be needed.
| if (InheritanceWithStrictVisibility) { | ||
| diag(MatchedFunction->getLocation(), | ||
| "visibility of function %0 is changed from %1 (through %1 " | ||
| "inheritance of class %2) to %3") |
Member
There was a problem hiding this comment.
consider adding "to %3 in class %4"
Collaborator
Author
There was a problem hiding this comment.
Will this not make the message too long? (The message is already located in the class %4.)
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
Outdated
Show resolved
Hide resolved
| // 'f_pub' would have private access but is forced to be | ||
| // public | ||
| }; | ||
|
|
Member
There was a problem hiding this comment.
what about when user changes visibility of the function with "using declaration" ?
|
|
||
| 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. |
Member
There was a problem hiding this comment.
Proxy functions are out of date, in current days "using declaration" can be used for that.
clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
Show resolved
Hide resolved
vbvictor
reviewed
May 17, 2025
Contributor
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
I think we should make change the name of the check to include "method" or "member-function" words in it. It will make clear that we check for methods visibility and not some visibility attributes on functions.
Possible names:
bugprone-member-function-visibility-change
bugprone-virtual-member-function-visibility-change
bugprone-visibility-change-to-virtual-member-function (imho the best)
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
8
to
10
| 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 |
Contributor
There was a problem hiding this comment.
Suggested change
| 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 | |
| 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 |
| // CHECK-MESSAGES: :[[@LINE-13]]:11: note: function declared here | ||
| }; | ||
|
|
||
| } |
Collaborator
Author
|
The current code is not finished. I want to check for false positives and probably add options to disable check at destructors and operators. |
Collaborator
Author
|
I have renamed the check, if OK the class name will be changed too (in a new commit). |
clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
Outdated
Show resolved
Hide resolved
vbvictor
reviewed
May 24, 2025
clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
Outdated
Show resolved
Hide resolved
| 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| Default value: empty string. | |
| Default value is empty string. |
HerrCai0907
reviewed
Jun 3, 2025
Contributor
HerrCai0907
left a comment
HerrCai0907
left a comment
There was a problem hiding this comment.
In my opinion, it should not be bugprone because I don't find any potential bug in this code style. Maybe it more like a readability issue? @PiotrZSL @EugeneZelenko what do you think?
Contributor
Sorry, I don't have strong opinion on this matter. |
Collaborator
Author
I think the "misc" module would be the best place for this check. |
clang-tools-extra/clang-tidy/misc/VisibilityChangeToVirtualFunctionCheck.cpp
Show resolved
Hide resolved
PiotrZSL
reviewed
Jul 24, 2025
Member
PiotrZSL
left a comment
PiotrZSL
left a comment
There was a problem hiding this comment.
Overall looks fine, the only thing is name.
Main problem is that this doesn't look to be bugprone or readability.
Misc is an generic category, sometimes I missing "design" category in clang-tidy, right now many checks that fall into same category are actually in cppguidelines namespace but there is not rule for this thing.
As for a name of the check, maybe something like "misc-virtual-function-visibility-change-in-override" to make it more clear.
| CheckDestructors ? decl() : decl(unless(cxxDestructorDecl())); | ||
| auto FilterOperators = | ||
| CheckOperators ? namedDecl() : namedDecl(unless(isOperatorDecl())); | ||
| Finder->addMatcher( |
Member
There was a problem hiding this comment.
Reduce matching only to in class definition/declaration, to avoid raising issue in both cpp and hpp, then move unless(isExpansionInSystemHeader()) to method level.
Collaborator
Author
There was a problem hiding this comment.
The check for MatchedFunction->isCanonicalDecl() already should remove multiple results from cpp and hpp file (the test file has tests for this).
Collaborator
Author
There was a problem hiding this comment.
Why is move of unless(isExpansionInSystemHeader()) needed?
clang-tools-extra/clang-tidy/misc/VisibilityChangeToVirtualFunctionCheck.cpp
Outdated
Show resolved
Hide resolved
| namespace clang::tidy { | ||
|
|
||
| template <> | ||
| struct OptionEnumMapping< |
Collaborator
Author
There was a problem hiding this comment.
It is not convenient because it uses things from the clang::tidy namespace. (In other checkers these mappings are in clang::tidy too.)
Contributor
Another name option: misc-consistent-virtual-function-visibility |
Contributor
|
The overall check implementation looks great. Once the name is settled, it is ready IMO. |
balazske
changed the title
balazske
changed the title
| Result.Nodes.getNodeAs<FunctionDecl>("base_func"); | ||
| const auto *BaseClass = Result.Nodes.getNodeAs<CXXRecordDecl>("base"); | ||
|
|
||
| AccessSpecifier ActualAccess = MatchedFunction->getAccess(); |
Contributor
There was a problem hiding this comment.
Suggested change
| AccessSpecifier ActualAccess = MatchedFunction->getAccess(); | |
| const AccessSpecifier ActualAccess = MatchedFunction->getAccess(); |
Collaborator
Author
There was a problem hiding this comment.
Local stack variables are usually not const-qualified in llvm code.
Contributor
There was a problem hiding this comment.
It's reasonable to highlight fact that they are not changed later in code. See also misc-const-correctness.
Contributor
There was a problem hiding this comment.
I'm looking forward to enable misc-const-correctness someday and hopefully write CI to enforce it.
Look at clang-tools-extra/clang-tidy/.clang-tidy, there are already many checks enabled, and we encourage devs to run clang-tidy over their code (or at least have it enabled in clangd). clang-tools-extra/clang-tidy dir must be 100% conformant with current config right now (unless someone pushed code with warning that we can not track)
Collaborator
Author
There was a problem hiding this comment.
What are the exact rules of using const on stack variables? I did not know about a rule of making all read-only variables const and this rule is not used in existing code of other checks (and other llvm files). The Coding Standards document does not tell anything about this.
Collaborator
Author
There was a problem hiding this comment.
In this single case I do not like to add const, or variables ParentClass, BaseClass, MatchedFunction, OverriddenFunction should be made const too.
Contributor
There was a problem hiding this comment.
Sure, please use const for other variables whose vales are not changed during lifetime. Obviously, this will improve code readability.
Contributor
|
Yet another suggestion for the name: As far as I see, currently the following name suggestions are active:
... but there were also three earlier @balazske @vbvictor @PiotrZSL @5chmidti @EugeneZelenko @ anybody else interested: |
Collaborator
Author
|
I like the names |
Contributor
|
I slightly don't like words consistent/inconsistent in check name because its meaning can vary from person to person. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
balazske
force-pushed
the
tidy_visibility_change_check
branch
from
d9a63dc to
181130f
Compare
balazske
changed the title
NagyDonat
approved these changes
Aug 15, 2025
Contributor
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
This commit still LGTM and it would be nice to get it merged.
@PiotrZSL @EugeneZelenko @vbvictor Are you satisfied with the current state of the commit? Do you have any concerns that are not answered yet?
vbvictor
approved these changes
Aug 15, 2025
Contributor
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
LGTM with couple small comments
|
|
||
| .. option:: CheckDestructors | ||
|
|
||
| If turned on, the check does apply to destructors too. Otherwise destructors |
Contributor
There was a problem hiding this comment.
Suggested change
| If turned on, the check does apply to destructors too. Otherwise destructors | |
| If `true`, the check does apply to destructors too. Otherwise destructors |
|
|
||
| .. option:: CheckOperators | ||
|
|
||
| If turned on, the check does apply to overloaded C++ operators (as virtual |
Contributor
There was a problem hiding this comment.
Suggested change
| If turned on, the check does apply to overloaded C++ operators (as virtual | |
| If `true`, the check does apply to overloaded C++ operators (as virtual |
| class Test1: public IgnoreSelected { | ||
| public: | ||
| void f(); | ||
| // CHECK-NOTES: :[[@LINE-1]]:8: warning: visibility of function 'f' |
Contributor
There was a problem hiding this comment.
I'm not sure why here we use CHECK-NOTES and everywhere else in CHECK-MESSAGES, I'd suggest to place CHECK-MESSAGES in this file.
| class B: public A { | ||
| public: | ||
| void f1(); | ||
| // CHECK-NOTES-WIDENING: :[[@LINE-1]]:8: warning: visibility of function 'f1' |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/5841 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.