https://github.com/t-a-james updated https://github.com/llvm/llvm-project/pull/154746
>From 39ad4945d05f3e88fd32dfdea585aa99c6ba985f Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Thu, 21 Aug 2025 13:14:24 +0100 Subject: [PATCH 01/16] [clang-tidy] New bugprone-method-hiding check --- .../bugprone/BugproneTidyModule.cpp | 6 + .../clang-tidy/bugprone/CMakeLists.txt | 7 + .../clang-tidy/bugprone/MethodHidingCheck.cpp | 148 ++++++++++++++++++ .../clang-tidy/bugprone/MethodHidingCheck.h | 37 +++++ clang-tools-extra/docs/ReleaseNotes.rst | 11 ++ .../checks/bugprone/method-hiding.rst | 15 ++ .../docs/clang-tidy/checks/list.rst | 7 + .../checkers/bugprone/method-hiding.cpp | 118 ++++++++++++++ 8 files changed, 349 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 824ebdfbd00dc..d1d1979794743 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -4,6 +4,9 @@ // See https://llvm.org/LICENSE.txt for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // +// SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates +// May 2025 modified by Siemens and/or its affiliates by Tom James +// //===----------------------------------------------------------------------===// #include "../ClangTidy.h" @@ -42,6 +45,7 @@ #include "LambdaFunctionNameCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" +#include "MethodHidingCheck.h" #include "MisleadingSetterOfReferenceCheck.h" #include "MisplacedOperatorInStrlenInAllocCheck.h" #include "MisplacedPointerArithmeticInAllocCheck.h" @@ -153,6 +157,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); + CheckFactories.registerCheck<MethodHidingCheck>( + "bugprone-method-hiding"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 59928e5e47a09..5745654855108 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -1,3 +1,9 @@ +# 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 +# SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates +# May 2025 modified by Siemens and/or its affiliates by Tom James + set(LLVM_LINK_COMPONENTS support FrontendOpenMP @@ -31,6 +37,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp + MethodHidingCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp SuspiciousStringviewDataUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp new file mode 100644 index 0000000000000..865ef115950e6 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -0,0 +1,148 @@ +//===--- MethodHidingCheck.cpp - clang-tidy ----------------------------===// +// +// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates +// +// 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 "MethodHidingCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include <stack> + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +bool sameBasicType(ParmVarDecl const *Lhs, ParmVarDecl const *Rhs) { + if (Lhs && Rhs) { + return Lhs->getType() + .getCanonicalType() + .getNonReferenceType() + .getUnqualifiedType() == Rhs->getType() + .getCanonicalType() + .getNonReferenceType() + .getUnqualifiedType(); + } + return false; +} + +bool namesCollide(CXXMethodDecl const &Lhs, CXXMethodDecl const &Rhs) { + if (Lhs.getNameAsString() != Rhs.getNameAsString()) { + return false; + } + if (Lhs.isConst() != Rhs.isConst()) { + return false; + } + if (Lhs.getNumParams() != Rhs.getNumParams()) { + return false; + } + for (unsigned int It = 0; It < Lhs.getNumParams(); ++It) { + if (!sameBasicType(Lhs.getParamDecl(It), Rhs.getParamDecl(It))) { + return false; + } + } + // Templates are not handled yet + if (Lhs.isTemplated() || Rhs.isTemplated()) { + return false; + } + if (Lhs.isTemplateInstantiation() || Rhs.isTemplateInstantiation()) { + return false; + } + if (Lhs.isFunctionTemplateSpecialization() || + Rhs.isFunctionTemplateSpecialization()) { + return false; + } + return true; +} + +AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { + CXXRecordDecl const *DerivedClass = Node.getParent(); + for (auto const &Base : DerivedClass->bases()) { + std::stack<CXXBaseSpecifier const *> Stack; + Stack.push(&Base); + while (!Stack.empty()) { + CXXBaseSpecifier const *CurrentBaseSpec = Stack.top(); + Stack.pop(); + + if (CurrentBaseSpec->getAccessSpecifier() == + clang::AccessSpecifier::AS_private) { + continue; + } + + const auto *CurrentRecord = + CurrentBaseSpec->getType()->getAsCXXRecordDecl(); + if (!CurrentRecord) { + continue; + } + + for (auto const &BaseMethod : CurrentRecord->methods()) { + if (namesCollide(*BaseMethod, Node)) { + ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder); + Builder->setBinding("base_method", + clang::DynTypedNode::create(*BaseMethod)); + return true; + } + } + + for (auto const &SubBase : CurrentRecord->bases()) { + Stack.push(&SubBase); + } + } + } + return false; +} + +// Same as clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp, +// similar matchers are used elsewhere in LLVM +AST_MATCHER(CXXMethodDecl, isOutOfLine) { return Node.isOutOfLine(); } + +} // namespace + +MethodHidingCheck::MethodHidingCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +void MethodHidingCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxMethodDecl( + unless(anyOf(isOutOfLine(), isStaticStorageClass(), isImplicit(), + cxxConstructorDecl(), isOverride(), + // isFinal(), //included with isOverride, + isPrivate())), + ofClass(cxxRecordDecl( + isDerivedFrom(cxxRecordDecl(unless(isInStdNamespace())))) + .bind("derived_class")), + nameCollidesWithMethodInBase()) + .bind("shadowing_method"), + this); +} + +void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { + auto const *ShadowingMethod = + Result.Nodes.getNodeAs<CXXMethodDecl>("shadowing_method"); + auto const *DerivedClass = + Result.Nodes.getNodeAs<CXXRecordDecl>("derived_class"); + auto const *BaseMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("base_method"); + + if (!ShadowingMethod || !DerivedClass || !BaseMethod) { + llvm_unreachable("Required binding not found"); + } + + auto const MethodName = ShadowingMethod->getNameInfo().getAsString(); + auto const DerivedClassName = DerivedClass->getNameAsString(); + std::string BaseClassName = BaseMethod->getParent()->getNameAsString(); + + std::string Message; + llvm::raw_string_ostream StringStream(Message); + StringStream << "'" << ShadowingMethod->getQualifiedNameAsString() + << "' hides same method in '" << BaseClassName << "'"; + diag(ShadowingMethod->getBeginLoc(), Message); + diag(BaseMethod->getBeginLoc(), "previous definition is here", + DiagnosticIDs::Note); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h new file mode 100644 index 0000000000000..f00f46cdf72e2 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h @@ -0,0 +1,37 @@ +//===--- MethodHidingCheck.h - clang-tidy --------------------*- C++ -*-===// +// +// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates +// +// 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_METHODHIDINGCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_METHODHIDINGCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Checks that a derived class does not define the same (non virtual) method as +/// a base class +/// +/// This anti-pattern is a Liskov violation. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/method-hiding.html +class MethodHidingCheck : public ClangTidyCheck { +public: + MethodHidingCheck(StringRef Name, ClangTidyContext *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_METHODHIDINGCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 4fee4f93908da..8147aae4a0201 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -1,6 +1,12 @@ .. If you want to modify sections/contents permanently, you should modify both ReleaseNotes.rst and ReleaseNotesTemplate.txt. +.. 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 +.. SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates +.. May 2025 modified by Siemens and/or its affiliates by Tom James + ==================================================== Extra Clang Tools |release| |ReleaseNotesTitle| ==================================================== @@ -135,6 +141,11 @@ New checks Detects default initialization (to 0) of variables with ``enum`` type where the enum has no enumerator with value of 0. +- New :doc:`bugprone-method-hiding + <clang-tidy/checks/bugprone/method-hiding>` check. + + Finds derived class methods that shadow a (non-virtual) base class method. + - New :doc:`cppcoreguidelines-pro-bounds-avoid-unchecked-container-access <clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst new file mode 100644 index 0000000000000..dd58ec29e60c2 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst @@ -0,0 +1,15 @@ +.. SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates +.. 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 + +.. title:: clang-tidy - bugprone-method-hiding + +bugprone-method-hiding +========================= + +Finds derived class methods that hide a (non-virtual) base class method. + +In order to be considered "hiding", methods must have the same signature +(i.e. the same name, same number of parameters, same parameter types, etc). +Only checks public, non-templated methods. \ No newline at end of file diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 5e3ffc4f8aca3..3a942c18c80b0 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -1,3 +1,9 @@ +.. 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 +.. SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates +.. May 2025 modified by Siemens and/or its affiliates by Tom James + .. title:: clang-tidy - Clang-Tidy Checks Clang-Tidy Checks @@ -110,6 +116,7 @@ Clang-Tidy Checks :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`, :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes" :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`, + :doc:`bugprone-method-hiding <bugprone/method-shadowing>` :doc:`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>`, :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes" :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp new file mode 100644 index 0000000000000..38511c8807611 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp @@ -0,0 +1,118 @@ +//===--- MethodHidingCheck.cpp - clang-tidy ----------------------------===// +// +// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates +// +// 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 +// +//===----------------------------------------------------------------------===// + +// RUN: %check_clang_tidy %s bugprone-method-hiding %t + + +class Base +{ + void method(); + void methodWithArg(int I); + + virtual Base* getThis() = 0; +}; + +class A : public Base +{ +public: + void method(); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'A::method' hides same method in 'Base' [bugprone-method-hiding] +}; + +// only declaration should be checked +void A::method() +{ +} + +class B +{ +public: + void method(); +}; + +class D: public Base +{ + +}; + +// test indirect inheritance +class E : public D +{ +public: + void method(); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'E::method' hides same method in 'Base' [bugprone-method-hiding] +}; + +class H : public Base +{ +public: + Base* getThis() override; + Base const* getThis() const; +}; + +class I : public Base +{ +public: + // test with inline implementation + void method() +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'I::method' hides same method in 'Base' [bugprone-method-hiding] + { + + } +}; + +class J : public Base +{ +public: + Base* getThis() final; +}; + +template<typename T> +class TemplateBase +{ +public: + virtual void size() const = 0; +}; + +template<typename T> +class K : public TemplateBase<T> +{ +public: + void size() const final; +}; + +class L : public Base +{ +public: +// not same signature (take const ref) but still ambiguous + void methodWithArg(int const& I); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-method-hiding] + + void methodWithArg(int const I); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-method-hiding] + + void methodWithArg(int *I); + void methodWithArg(int const* I); +}; + +class M : public Base +{ +public: + static void method(); +}; + +class N : public Base +{ +public: + template<typename T> + void methodWithArg(T I); + // TODO: Templates are not handled yet + template<> void methodWithArg<int>(int I); +}; >From b84fb6ce2ea527a9126e0a1f5b47b4c967dc43d9 Mon Sep 17 00:00:00 2001 From: Tom <41913303+t-a-ja...@users.noreply.github.com> Date: Fri, 22 Aug 2025 09:40:34 +0100 Subject: [PATCH 02/16] Update clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index 865ef115950e6..52e2c4a9d2626 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -1,4 +1,4 @@ -//===--- MethodHidingCheck.cpp - clang-tidy ----------------------------===// +//===----------------------------------------------------------------------===// // // SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates // >From 1fe1d5bc7944da1d3ab5fa9c71cf297346536427 Mon Sep 17 00:00:00 2001 From: Tom <41913303+t-a-ja...@users.noreply.github.com> Date: Fri, 22 Aug 2025 09:40:44 +0100 Subject: [PATCH 03/16] Update clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h index f00f46cdf72e2..1ea5d656a67b9 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h @@ -1,4 +1,4 @@ -//===--- MethodHidingCheck.h - clang-tidy --------------------*- C++ -*-===// +//===----------------------------------------------------------------------===// // // SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates // >From 316053382d50bbb0328d1b6216c132cc044b347b Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Tue, 26 Aug 2025 11:07:35 +0100 Subject: [PATCH 04/16] Auto-format BugproneTidyModule.cpp --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index d1d1979794743..950a0410edfdb 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -157,8 +157,7 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); - CheckFactories.registerCheck<MethodHidingCheck>( - "bugprone-method-hiding"); + CheckFactories.registerCheck<MethodHidingCheck>("bugprone-method-hiding"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( >From e67dedae954c96ffa485f595543741ddbb6becaa Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Tue, 26 Aug 2025 11:16:56 +0100 Subject: [PATCH 05/16] Remove unwanted copyright messages --- .../clang-tidy/bugprone/BugproneTidyModule.cpp | 3 --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt | 6 ------ .../clang-tidy/bugprone/MethodHidingCheck.cpp | 2 -- .../clang-tidy/bugprone/MethodHidingCheck.h | 2 -- clang-tools-extra/docs/ReleaseNotes.rst | 6 ------ .../docs/clang-tidy/checks/bugprone/method-hiding.rst | 5 ----- clang-tools-extra/docs/clang-tidy/checks/list.rst | 6 ------ .../clang-tidy/checkers/bugprone/method-hiding.cpp | 11 ----------- 8 files changed, 41 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 950a0410edfdb..ac59b049ef46a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -4,9 +4,6 @@ // See https://llvm.org/LICENSE.txt for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // -// SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates -// May 2025 modified by Siemens and/or its affiliates by Tom James -// //===----------------------------------------------------------------------===// #include "../ClangTidy.h" diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 5745654855108..e2a2d3397902f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -1,9 +1,3 @@ -# 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 -# SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates -# May 2025 modified by Siemens and/or its affiliates by Tom James - set(LLVM_LINK_COMPONENTS support FrontendOpenMP diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index 52e2c4a9d2626..c5bc8ca24d617 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -1,7 +1,5 @@ //===----------------------------------------------------------------------===// // -// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates -// // 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 diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h index 1ea5d656a67b9..f813e6d3f8ba5 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h @@ -1,7 +1,5 @@ //===----------------------------------------------------------------------===// // -// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates -// // 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8147aae4a0201..e7b02e51efc5a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -1,12 +1,6 @@ .. If you want to modify sections/contents permanently, you should modify both ReleaseNotes.rst and ReleaseNotesTemplate.txt. -.. 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 -.. SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates -.. May 2025 modified by Siemens and/or its affiliates by Tom James - ==================================================== Extra Clang Tools |release| |ReleaseNotesTitle| ==================================================== diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst index dd58ec29e60c2..56d0ff71130d8 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst @@ -1,8 +1,3 @@ -.. SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates -.. 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 - .. title:: clang-tidy - bugprone-method-hiding bugprone-method-hiding diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 3a942c18c80b0..79fb3d70684db 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -1,9 +1,3 @@ -.. 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 -.. SPDX-FileCopyrightText: Portions Copyright 2025 Siemens and/or its affiliates -.. May 2025 modified by Siemens and/or its affiliates by Tom James - .. title:: clang-tidy - Clang-Tidy Checks Clang-Tidy Checks diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp index 38511c8807611..8aa5557dcd192 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp @@ -1,16 +1,5 @@ -//===--- MethodHidingCheck.cpp - clang-tidy ----------------------------===// -// -// SPDX-FileCopyrightText: 2025 Siemens Corporation and/or its affiliates -// -// 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 -// -//===----------------------------------------------------------------------===// - // RUN: %check_clang_tidy %s bugprone-method-hiding %t - class Base { void method(); >From bd53bccd126954a8a7ed6c652b33003330dc6536 Mon Sep 17 00:00:00 2001 From: Tom <41913303+t-a-ja...@users.noreply.github.com> Date: Tue, 26 Aug 2025 15:36:48 +0100 Subject: [PATCH 06/16] Update clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index c5bc8ca24d617..1e8aec337f47c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -132,7 +132,7 @@ void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { auto const MethodName = ShadowingMethod->getNameInfo().getAsString(); auto const DerivedClassName = DerivedClass->getNameAsString(); - std::string BaseClassName = BaseMethod->getParent()->getNameAsString(); + const std::string BaseClassName = BaseMethod->getParent()->getNameAsString(); std::string Message; llvm::raw_string_ostream StringStream(Message); >From e9157961bc29754840826d7dece7fe908cae8a74 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Tue, 26 Aug 2025 15:46:41 +0100 Subject: [PATCH 07/16] Remove unused variables --- .../clang-tidy/bugprone/MethodHidingCheck.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index 1e8aec337f47c..3b27cd0637d12 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -130,15 +130,10 @@ void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { llvm_unreachable("Required binding not found"); } - auto const MethodName = ShadowingMethod->getNameInfo().getAsString(); - auto const DerivedClassName = DerivedClass->getNameAsString(); - const std::string BaseClassName = BaseMethod->getParent()->getNameAsString(); - - std::string Message; - llvm::raw_string_ostream StringStream(Message); - StringStream << "'" << ShadowingMethod->getQualifiedNameAsString() - << "' hides same method in '" << BaseClassName << "'"; - diag(ShadowingMethod->getBeginLoc(), Message); + diag(ShadowingMethod->getBeginLoc(), + "'" + ShadowingMethod->getQualifiedNameAsString() + + "' hides same method in '" + + BaseMethod->getParent()->getNameAsString() + "'"); diag(BaseMethod->getBeginLoc(), "previous definition is here", DiagnosticIDs::Note); } >From d22bfa404e79e4f566fb27a975c66a2166c635d1 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 09:00:38 +0100 Subject: [PATCH 08/16] Remove braces --- .../clang-tidy/bugprone/MethodHidingCheck.cpp | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index 3b27cd0637d12..e96b74428ba94 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -30,31 +30,23 @@ bool sameBasicType(ParmVarDecl const *Lhs, ParmVarDecl const *Rhs) { } bool namesCollide(CXXMethodDecl const &Lhs, CXXMethodDecl const &Rhs) { - if (Lhs.getNameAsString() != Rhs.getNameAsString()) { + if (Lhs.getNameAsString() != Rhs.getNameAsString()) return false; - } - if (Lhs.isConst() != Rhs.isConst()) { + if (Lhs.isConst() != Rhs.isConst()) return false; - } - if (Lhs.getNumParams() != Rhs.getNumParams()) { + if (Lhs.getNumParams() != Rhs.getNumParams()) return false; - } - for (unsigned int It = 0; It < Lhs.getNumParams(); ++It) { - if (!sameBasicType(Lhs.getParamDecl(It), Rhs.getParamDecl(It))) { + for (unsigned int It = 0; It < Lhs.getNumParams(); ++It) + if (!sameBasicType(Lhs.getParamDecl(It), Rhs.getParamDecl(It))) return false; - } - } // Templates are not handled yet - if (Lhs.isTemplated() || Rhs.isTemplated()) { + if (Lhs.isTemplated() || Rhs.isTemplated()) return false; - } - if (Lhs.isTemplateInstantiation() || Rhs.isTemplateInstantiation()) { + if (Lhs.isTemplateInstantiation() || Rhs.isTemplateInstantiation()) return false; - } if (Lhs.isFunctionTemplateSpecialization() || - Rhs.isFunctionTemplateSpecialization()) { + Rhs.isFunctionTemplateSpecialization()) return false; - } return true; } @@ -68,15 +60,13 @@ AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { Stack.pop(); if (CurrentBaseSpec->getAccessSpecifier() == - clang::AccessSpecifier::AS_private) { + clang::AccessSpecifier::AS_private) continue; - } const auto *CurrentRecord = CurrentBaseSpec->getType()->getAsCXXRecordDecl(); - if (!CurrentRecord) { + if (!CurrentRecord) continue; - } for (auto const &BaseMethod : CurrentRecord->methods()) { if (namesCollide(*BaseMethod, Node)) { @@ -87,9 +77,9 @@ AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { } } - for (auto const &SubBase : CurrentRecord->bases()) { + for (auto const &SubBase : CurrentRecord->bases()) Stack.push(&SubBase); - } + } } return false; @@ -126,9 +116,8 @@ void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { Result.Nodes.getNodeAs<CXXRecordDecl>("derived_class"); auto const *BaseMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("base_method"); - if (!ShadowingMethod || !DerivedClass || !BaseMethod) { + if (!ShadowingMethod || !DerivedClass || !BaseMethod) llvm_unreachable("Required binding not found"); - } diag(ShadowingMethod->getBeginLoc(), "'" + ShadowingMethod->getQualifiedNameAsString() + >From 19465b6b2c615f4f9e394b97f17118bf7afc9093 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 09:48:30 +0100 Subject: [PATCH 09/16] Give an example in the documentation --- .../checks/bugprone/method-hiding.rst | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst index 56d0ff71130d8..a4a2436118b44 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst @@ -7,4 +7,24 @@ Finds derived class methods that hide a (non-virtual) base class method. In order to be considered "hiding", methods must have the same signature (i.e. the same name, same number of parameters, same parameter types, etc). -Only checks public, non-templated methods. \ No newline at end of file +Only checks public, non-templated methods. + +The below example is bugprone because consumers of the `Derived` class will +expect the `reset` method to do the work of `Base::reset()` in addition to extra +work required to reset the `Derived` class. Common fixes include: +- Making the `reset` method polymorphic +- Re-naming `Derived::reset` if it's not meant to intersect with `Base::reset` +- Using `using Base::reset` to change the access specifier + +This is also a violation of the Liskov Substitution Principle. + +.. code-block:: c++ + + class Base { + void reset() {/* reset the base class */}; + }; + + class Derived : public Base { + public: + void reset() {/* reset the derived class, but not the base class */}; + }; \ No newline at end of file >From d337e8206eb78d47d7122dde41310e3e1ba0277c Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 09:52:43 +0100 Subject: [PATCH 10/16] West const everywhere --- .../clang-tidy/bugprone/MethodHidingCheck.cpp | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index e96b74428ba94..edcd929c3e2e5 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -16,7 +16,7 @@ namespace clang::tidy::bugprone { namespace { -bool sameBasicType(ParmVarDecl const *Lhs, ParmVarDecl const *Rhs) { +bool sameBasicType(const ParmVarDecl *Lhs, const ParmVarDecl *Rhs) { if (Lhs && Rhs) { return Lhs->getType() .getCanonicalType() @@ -29,7 +29,7 @@ bool sameBasicType(ParmVarDecl const *Lhs, ParmVarDecl const *Rhs) { return false; } -bool namesCollide(CXXMethodDecl const &Lhs, CXXMethodDecl const &Rhs) { +bool namesCollide(const CXXMethodDecl &Lhs, const CXXMethodDecl &Rhs) { if (Lhs.getNameAsString() != Rhs.getNameAsString()) return false; if (Lhs.isConst() != Rhs.isConst()) @@ -51,12 +51,12 @@ bool namesCollide(CXXMethodDecl const &Lhs, CXXMethodDecl const &Rhs) { } AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { - CXXRecordDecl const *DerivedClass = Node.getParent(); - for (auto const &Base : DerivedClass->bases()) { - std::stack<CXXBaseSpecifier const *> Stack; + const CXXRecordDecl *DerivedClass = Node.getParent(); + for (const auto &Base : DerivedClass->bases()) { + std::stack<const CXXBaseSpecifier *> Stack; Stack.push(&Base); while (!Stack.empty()) { - CXXBaseSpecifier const *CurrentBaseSpec = Stack.top(); + const CXXBaseSpecifier *CurrentBaseSpec = Stack.top(); Stack.pop(); if (CurrentBaseSpec->getAccessSpecifier() == @@ -68,7 +68,7 @@ AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { if (!CurrentRecord) continue; - for (auto const &BaseMethod : CurrentRecord->methods()) { + for (const auto &BaseMethod : CurrentRecord->methods()) { if (namesCollide(*BaseMethod, Node)) { ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder); Builder->setBinding("base_method", @@ -77,9 +77,8 @@ AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) { } } - for (auto const &SubBase : CurrentRecord->bases()) + for (const auto &SubBase : CurrentRecord->bases()) Stack.push(&SubBase); - } } return false; @@ -110,11 +109,11 @@ void MethodHidingCheck::registerMatchers(MatchFinder *Finder) { } void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { - auto const *ShadowingMethod = + const auto *ShadowingMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("shadowing_method"); - auto const *DerivedClass = + const auto *DerivedClass = Result.Nodes.getNodeAs<CXXRecordDecl>("derived_class"); - auto const *BaseMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("base_method"); + const auto *BaseMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("base_method"); if (!ShadowingMethod || !DerivedClass || !BaseMethod) llvm_unreachable("Required binding not found"); >From 02f230a80c12683d4ac104e7283e95a8ddd64f20 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 16:03:41 +0100 Subject: [PATCH 11/16] Improve diagnostics --- .../clang-tidy/bugprone/MethodHidingCheck.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index edcd929c3e2e5..078161778d678 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -118,12 +118,12 @@ void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { if (!ShadowingMethod || !DerivedClass || !BaseMethod) llvm_unreachable("Required binding not found"); - diag(ShadowingMethod->getBeginLoc(), - "'" + ShadowingMethod->getQualifiedNameAsString() + - "' hides same method in '" + - BaseMethod->getParent()->getNameAsString() + "'"); - diag(BaseMethod->getBeginLoc(), "previous definition is here", - DiagnosticIDs::Note); + diag(ShadowingMethod->getBeginLoc(), "'%0' hides same method in '%1'") + << ShadowingMethod->getQualifiedNameAsString() + << BaseMethod->getParent()->getNameAsString(); + diag(BaseMethod->getBeginLoc(), "previous definition of '%0' is here", + DiagnosticIDs::Note) + << ShadowingMethod->getNameAsString(); } } // namespace clang::tidy::bugprone >From c5e31e3ca1bc00713419776c0dd296ce9040555b Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 16:12:20 +0100 Subject: [PATCH 12/16] set TK_IgnoreUnlessSpelledInSource --- clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h index f813e6d3f8ba5..111377826411d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h @@ -28,6 +28,9 @@ class MethodHidingCheck : public ClangTidyCheck { bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus; } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } }; } // namespace clang::tidy::bugprone >From 30ca72238c37b992fc76604eed90523a2436e0c2 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Wed, 27 Aug 2025 16:20:49 +0100 Subject: [PATCH 13/16] Fix docs --- .../checks/bugprone/method-hiding.rst | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst index a4a2436118b44..a4ad7aafb5736 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst @@ -9,22 +9,21 @@ In order to be considered "hiding", methods must have the same signature (i.e. the same name, same number of parameters, same parameter types, etc). Only checks public, non-templated methods. -The below example is bugprone because consumers of the `Derived` class will -expect the `reset` method to do the work of `Base::reset()` in addition to extra -work required to reset the `Derived` class. Common fixes include: -- Making the `reset` method polymorphic -- Re-naming `Derived::reset` if it's not meant to intersect with `Base::reset` -- Using `using Base::reset` to change the access specifier +The below example is bugprone because consumers of the ``Derived`` class will +expect the ``reset`` method to do the work of ``Base::reset()`` in addition to extra +work required to reset the ``Derived`` class. Common fixes include: +- Making the ``reset`` method polymorphic +- Re-naming ``Derived::reset`` if it's not meant to intersect with ``Base::reset`` +- Using ``using Base::reset`` to change the access specifier This is also a violation of the Liskov Substitution Principle. .. code-block:: c++ - class Base { + struct Base { void reset() {/* reset the base class */}; }; - class Derived : public Base { - public: + struct Derived : public Base { void reset() {/* reset the derived class, but not the base class */}; }; \ No newline at end of file >From f91f8948c61588541d5b91b0bf4cbc36a77eef16 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Thu, 28 Aug 2025 14:43:16 +0100 Subject: [PATCH 14/16] Move template logic to the matcher --- .../clang-tidy/bugprone/MethodHidingCheck.cpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp index 078161778d678..c73700f61fe2f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp @@ -8,6 +8,7 @@ #include "MethodHidingCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include <stack> using namespace clang::ast_matchers; @@ -39,14 +40,6 @@ bool namesCollide(const CXXMethodDecl &Lhs, const CXXMethodDecl &Rhs) { for (unsigned int It = 0; It < Lhs.getNumParams(); ++It) if (!sameBasicType(Lhs.getParamDecl(It), Rhs.getParamDecl(It))) return false; - // Templates are not handled yet - if (Lhs.isTemplated() || Rhs.isTemplated()) - return false; - if (Lhs.isTemplateInstantiation() || Rhs.isTemplateInstantiation()) - return false; - if (Lhs.isFunctionTemplateSpecialization() || - Rhs.isFunctionTemplateSpecialization()) - return false; return true; } @@ -97,9 +90,11 @@ void MethodHidingCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( cxxMethodDecl( unless(anyOf(isOutOfLine(), isStaticStorageClass(), isImplicit(), - cxxConstructorDecl(), isOverride(), + cxxConstructorDecl(), isOverride(), isPrivate(), // isFinal(), //included with isOverride, - isPrivate())), + // Templates are not handled yet + ast_matchers::isTemplateInstantiation(), + ast_matchers::isExplicitTemplateSpecialization())), ofClass(cxxRecordDecl( isDerivedFrom(cxxRecordDecl(unless(isInStdNamespace())))) .bind("derived_class")), >From c376c45972bbda68f1a6ac115496abcd6321d455 Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Thu, 28 Aug 2025 14:52:18 +0100 Subject: [PATCH 15/16] Fix the documentation build --- .../docs/clang-tidy/checks/bugprone/method-hiding.rst | 1 + clang-tools-extra/docs/clang-tidy/checks/list.rst | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst index a4ad7aafb5736..2e57ab89bfa02 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst @@ -12,6 +12,7 @@ Only checks public, non-templated methods. The below example is bugprone because consumers of the ``Derived`` class will expect the ``reset`` method to do the work of ``Base::reset()`` in addition to extra work required to reset the ``Derived`` class. Common fixes include: + - Making the ``reset`` method polymorphic - Re-naming ``Derived::reset`` if it's not meant to intersect with ``Base::reset`` - Using ``using Base::reset`` to change the access specifier diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 79fb3d70684db..0f95358da624b 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -110,7 +110,7 @@ Clang-Tidy Checks :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`, :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes" :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`, - :doc:`bugprone-method-hiding <bugprone/method-shadowing>` + :doc:`bugprone-method-hiding <bugprone/method-hiding>` :doc:`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>`, :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes" :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" >From 899cd2ccc6ad9941599b50a6b24ab9f6e50d348f Mon Sep 17 00:00:00 2001 From: Tom James <tom.ja...@siemens.com> Date: Thu, 28 Aug 2025 15:06:06 +0100 Subject: [PATCH 16/16] Re-name to bugprone-derived-method-shadowing-base-method --- .../clang-tidy/bugprone/BugproneTidyModule.cpp | 5 +++-- .../clang-tidy/bugprone/CMakeLists.txt | 2 +- ...pp => DerivedMethodShadowingBaseMethodCheck.cpp} | 11 +++++++---- ...ck.h => DerivedMethodShadowingBaseMethodCheck.h} | 13 +++++++------ clang-tools-extra/docs/ReleaseNotes.rst | 4 ++-- ...rst => derived-method-shadowing-base-method.rst} | 10 +++++----- clang-tools-extra/docs/clang-tidy/checks/list.rst | 4 ++-- ...cpp => derived-method-shadowing-base-method.cpp} | 12 ++++++------ 8 files changed, 33 insertions(+), 28 deletions(-) rename clang-tools-extra/clang-tidy/bugprone/{MethodHidingCheck.cpp => DerivedMethodShadowingBaseMethodCheck.cpp} (92%) rename clang-tools-extra/clang-tidy/bugprone/{MethodHidingCheck.h => DerivedMethodShadowingBaseMethodCheck.h} (65%) rename clang-tools-extra/docs/clang-tidy/checks/bugprone/{method-hiding.rst => derived-method-shadowing-base-method.rst} (72%) rename clang-tools-extra/test/clang-tidy/checkers/bugprone/{method-hiding.cpp => derived-method-shadowing-base-method.cpp} (79%) diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index ac59b049ef46a..87c4578de4fac 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -23,6 +23,7 @@ #include "CopyConstructorInitCheck.h" #include "CrtpConstructorAccessibilityCheck.h" #include "DanglingHandleCheck.h" +#include "DerivedMethodShadowingBaseMethodCheck.h" #include "DynamicStaticInitializersCheck.h" #include "EasilySwappableParametersCheck.h" #include "EmptyCatchCheck.h" @@ -42,7 +43,6 @@ #include "LambdaFunctionNameCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" -#include "MethodHidingCheck.h" #include "MisleadingSetterOfReferenceCheck.h" #include "MisplacedOperatorInStrlenInAllocCheck.h" #include "MisplacedPointerArithmeticInAllocCheck.h" @@ -154,7 +154,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); - CheckFactories.registerCheck<MethodHidingCheck>("bugprone-method-hiding"); + CheckFactories.registerCheck<DerivedMethodShadowingBaseMethodCheck>( + "bugprone-derived-method-shadowing-base-method"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e2a2d3397902f..2cd410c4093ce 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -31,7 +31,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp - MethodHidingCheck.cpp + DerivedMethodShadowingBaseMethodCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp SuspiciousStringviewDataUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.cpp similarity index 92% rename from clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp rename to clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.cpp index c73700f61fe2f..ed745584e232b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.cpp @@ -6,7 +6,7 @@ // //===----------------------------------------------------------------------===// -#include "MethodHidingCheck.h" +#include "DerivedMethodShadowingBaseMethodCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" #include <stack> @@ -83,10 +83,12 @@ AST_MATCHER(CXXMethodDecl, isOutOfLine) { return Node.isOutOfLine(); } } // namespace -MethodHidingCheck::MethodHidingCheck(StringRef Name, ClangTidyContext *Context) +DerivedMethodShadowingBaseMethodCheck::DerivedMethodShadowingBaseMethodCheck( + StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} -void MethodHidingCheck::registerMatchers(MatchFinder *Finder) { +void DerivedMethodShadowingBaseMethodCheck::registerMatchers( + MatchFinder *Finder) { Finder->addMatcher( cxxMethodDecl( unless(anyOf(isOutOfLine(), isStaticStorageClass(), isImplicit(), @@ -103,7 +105,8 @@ void MethodHidingCheck::registerMatchers(MatchFinder *Finder) { this); } -void MethodHidingCheck::check(const MatchFinder::MatchResult &Result) { +void DerivedMethodShadowingBaseMethodCheck::check( + const MatchFinder::MatchResult &Result) { const auto *ShadowingMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("shadowing_method"); const auto *DerivedClass = diff --git a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h b/clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.h similarity index 65% rename from clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h rename to clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.h index 111377826411d..a1609d08f7430 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MethodHidingCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.h @@ -6,8 +6,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_METHODHIDINGCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_METHODHIDINGCHECK_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H #include "../ClangTidyCheck.h" @@ -19,10 +19,11 @@ namespace clang::tidy::bugprone { /// This anti-pattern is a Liskov violation. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/method-hiding.html -class MethodHidingCheck : public ClangTidyCheck { +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.html +class DerivedMethodShadowingBaseMethodCheck : public ClangTidyCheck { public: - MethodHidingCheck(StringRef Name, ClangTidyContext *Context); + DerivedMethodShadowingBaseMethodCheck(StringRef Name, + ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { @@ -35,4 +36,4 @@ class MethodHidingCheck : public ClangTidyCheck { } // namespace clang::tidy::bugprone -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_METHODHIDINGCHECK_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e7b02e51efc5a..28993394852f9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -135,8 +135,8 @@ New checks Detects default initialization (to 0) of variables with ``enum`` type where the enum has no enumerator with value of 0. -- New :doc:`bugprone-method-hiding - <clang-tidy/checks/bugprone/method-hiding>` check. +- New :doc:`bugprone-derived-method-shadowing-base-method + <clang-tidy/checks/bugprone/derived-method-shadowing-base-method>` check. Finds derived class methods that shadow a (non-virtual) base class method. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.rst similarity index 72% rename from clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst rename to clang-tools-extra/docs/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.rst index 2e57ab89bfa02..f544abc14ffbf 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/method-hiding.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.rst @@ -1,11 +1,11 @@ -.. title:: clang-tidy - bugprone-method-hiding +.. title:: clang-tidy - bugprone-derived-method-shadowing-base-method -bugprone-method-hiding -========================= +bugprone-derived-method-shadowing-base-method +============================================= -Finds derived class methods that hide a (non-virtual) base class method. +Finds derived class methods that shadow a (non-virtual) base class method. -In order to be considered "hiding", methods must have the same signature +In order to be considered "shadowing", methods must have the same signature (i.e. the same name, same number of parameters, same parameter types, etc). Only checks public, non-templated methods. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0f95358da624b..726652727878a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -91,6 +91,7 @@ Clang-Tidy Checks :doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes" :doc:`bugprone-crtp-constructor-accessibility <bugprone/crtp-constructor-accessibility>`, "Yes" :doc:`bugprone-dangling-handle <bugprone/dangling-handle>`, + :doc:`bugprone-derived-method-shadowing-base-method <bugprone/derived-method-shadowing-base-method>`, :doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`, :doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`, :doc:`bugprone-empty-catch <bugprone/empty-catch>`, @@ -110,7 +111,6 @@ Clang-Tidy Checks :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`, :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes" :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`, - :doc:`bugprone-method-hiding <bugprone/method-hiding>` :doc:`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>`, :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes" :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" @@ -250,12 +250,12 @@ Clang-Tidy Checks :doc:`linuxkernel-must-check-errs <linuxkernel/must-check-errs>`, :doc:`llvm-header-guard <llvm/header-guard>`, :doc:`llvm-include-order <llvm/include-order>`, "Yes" - :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes" :doc:`llvm-namespace-comment <llvm/namespace-comment>`, :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm/prefer-isa-or-dyn-cast-in-conditionals>`, "Yes" :doc:`llvm-prefer-register-over-unsigned <llvm/prefer-register-over-unsigned>`, "Yes" :doc:`llvm-prefer-static-over-anonymous-namespace <llvm/prefer-static-over-anonymous-namespace>`, :doc:`llvm-twine-local <llvm/twine-local>`, "Yes" + :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes" :doc:`llvmlibc-callee-namespace <llvmlibc/callee-namespace>`, :doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`, :doc:`llvmlibc-inline-function-decl <llvmlibc/inline-function-decl>`, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/derived-method-shadowing-base-method.cpp similarity index 79% rename from clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/derived-method-shadowing-base-method.cpp index 8aa5557dcd192..1e7f5077b9056 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/method-hiding.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/derived-method-shadowing-base-method.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s bugprone-method-hiding %t +// RUN: %check_clang_tidy %s bugprone-derived-method-shadowing-base-method %t class Base { @@ -12,7 +12,7 @@ class A : public Base { public: void method(); -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'A::method' hides same method in 'Base' [bugprone-method-hiding] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'A::method' hides same method in 'Base' [bugprone-derived-method-shadowing-base-method] }; // only declaration should be checked @@ -36,7 +36,7 @@ class E : public D { public: void method(); -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'E::method' hides same method in 'Base' [bugprone-method-hiding] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'E::method' hides same method in 'Base' [bugprone-derived-method-shadowing-base-method] }; class H : public Base @@ -51,7 +51,7 @@ class I : public Base public: // test with inline implementation void method() -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'I::method' hides same method in 'Base' [bugprone-method-hiding] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'I::method' hides same method in 'Base' [bugprone-derived-method-shadowing-base-method] { } @@ -82,10 +82,10 @@ class L : public Base public: // not same signature (take const ref) but still ambiguous void methodWithArg(int const& I); -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-method-hiding] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-derived-method-shadowing-base-method] void methodWithArg(int const I); -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-method-hiding] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' hides same method in 'Base' [bugprone-derived-method-shadowing-base-method] void methodWithArg(int *I); void methodWithArg(int const* I); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
