diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -18,6 +18,7 @@ #include "CopyConstructorInitCheck.h" #include "DanglingHandleCheck.h" #include "DynamicStaticInitializersCheck.h" +#include "EasilySwappableParametersCheck.h" #include "ExceptionEscapeCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" @@ -89,6 +90,8 @@ "bugprone-dangling-handle"); CheckFactories.registerCheck( "bugprone-dynamic-static-initializers"); + CheckFactories.registerCheck( + "bugprone-easily-swappable-parameters"); CheckFactories.registerCheck( "bugprone-exception-escape"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -13,6 +13,7 @@ CopyConstructorInitCheck.cpp DanglingHandleCheck.cpp DynamicStaticInitializersCheck.cpp + EasilySwappableParametersCheck.cpp ExceptionEscapeCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h @@ -0,0 +1,47 @@ +//===--- EasilySwappableParametersCheck.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_EASILYSWAPPABLEPARAMETERSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EASILYSWAPPABLEPARAMETERSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds function definitions where parameters of convertible types follow +/// each other directly, making call sites prone to calling the function with +/// swapped (or badly ordered) arguments. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-easily-swappable-parameters.html +class EasilySwappableParametersCheck : public ClangTidyCheck { +public: + EasilySwappableParametersCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + + /// The minimum length of an adjacent swappable parameter range required for + /// a diagnostic. + const std::size_t MinimumLength; + + /// The parameter names (as written in the source text) to be ignored. + const std::vector IgnoredParameterNames; + + /// The parameter typename suffixes (as written in the source code) to be + /// ignored. + const std::vector IgnoredParameterTypeSuffixes; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EASILYSWAPPABLEPARAMETERSCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp @@ -0,0 +1,415 @@ +//===--- EasilySwappableParametersCheck.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 "EasilySwappableParametersCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +#define DEBUG_TYPE "EasilySwappableParametersCheck" +#include "llvm/Support/Debug.h" + +#ifndef NDEBUG + +template +static inline std::string formatBitsImpl(std::size_t Num) { + constexpr std::size_t WidthWithPadding = Width + (Width / 4); + char S[WidthWithPadding]; + for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) { + if (CurPos % 5 == 0) { + S[CurPos] = ' '; + continue; + } + + S[CurPos] = Num % 2 ? '1' : '0'; + Num >>= 1; + } + + return std::string(std::rbegin(S), std::rend(S)); +} + +/// Formats the bit representation of a numeric type as a string in groups of 4. +template static inline std::string formatBits(T &&V) { + return formatBitsImpl(V); +} + +#else + +template static inline std::string formatBits(T &&V); + +#endif + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +using namespace utils::options; + +namespace bugprone { + +using TheCheck = EasilySwappableParametersCheck; + +namespace { + +namespace filter { +bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node); +} // namespace filter + +namespace model { + +/// The language features involved in allowing the mix between two parameters. +enum MixFlags : unsigned char { + MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE! + +#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) + + FB(None, 1), //< Mix between the two parameters is not possible. + FB(Trivial, 2), //< The two mix trivially, and are the exact same type. + FB(Canonical, 3), //< The two mix because the types refer to the same + // CanonicalType, but we do not elaborate as to how. + +#undef FB + + LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/MIX_Canonical) +}; +LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); + +/// Contains the metadata for the mixability result between two types, +/// independently of which parameters they were calculated from. +struct MixData { + MixFlags Flags; + + MixData(MixFlags Flags) : Flags(Flags) {} + + void sanitise() { + assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec"); + // TODO: There will be statements here in further extensions of the check. + } +}; + +/// A named tuple that contains the information for a mix between two concrete +/// parameters. +struct Mix { + const ParmVarDecl *First, *Second; + MixData Data; + + Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data) + : First(F), Second(S), Data(std::move(Data)) {} + + void sanitise() { Data.sanitise(); } + MixFlags flags() const { return Data.Flags; } +}; + +static_assert(std::is_trivially_copyable::value && + std::is_trivially_move_constructible::value && + std::is_trivially_move_assignable::value, + "Keep frequently used data simple!"); + +struct MixableParameterRange { + /// The number of parameters iterated to build the instance. + std::size_t NumParamsChecked = 0; + + /// The individual flags and supporting information for the mixes. + SmallVector Mixes; + + /// Gets the leftmost parameter of the range. + const ParmVarDecl *getFirstParam() const { + // The first element is the LHS of the very first mix in the range. + assert(!Mixes.empty()); + return Mixes.front().First; + } + + /// Gets the rightmost parameter of the range. + const ParmVarDecl *getLastParam() const { + // The builder function breaks building an instance of this type if it + // finds something that can not be mixed with the rest, by going *forward* + // in the list of parameters. So at any moment of break, the RHS of the last + // element of the mix vector is also the last element of the mixing range. + assert(!Mixes.empty()); + return Mixes.back().Second; + } +}; + +/// Approximate the way how LType and RType might refer to "essentially the +/// same" type, in a sense that at a particular call site, an expression of +/// type LType and RType might be successfully passed to a variable (in our +/// specific case, a parameter) of type RType and LType, respectively. +/// Note the swapped order! +/// +/// The returned data structure is not guaranteed to be properly set, as this +/// function is potentially recursive. It is the caller's responsibility to +/// call sanitise() on the result once the recursion is over. +MixData calculateMixability(const TheCheck &Check, const QualType LType, + const QualType RType, const ASTContext &Ctx) { + LLVM_DEBUG(llvm::dbgs() << ">>> calculateMixability for LType:\n"; + LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n"; + RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';); + + if (LType == RType) { + LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Trivial equality.\n"); + return {MIX_Trivial}; + } + + // TODO: Implement more elaborate logic, such as typedef, implicit + // conversions, etc. + + // If none of the previous logic found a match, try if Clang otherwise + // believes the types to be the same. + if (LType.getCanonicalType() == RType.getCanonicalType()) { + LLVM_DEBUG(llvm::dbgs() + << "<<< calculateMixability. Same CanonicalType.\n"); + return {MIX_Canonical}; + } + + LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. No match found.\n"); + return {MIX_None}; +} + +MixableParameterRange modelMixingRange(const TheCheck &Check, + const FunctionDecl *FD, + const std::size_t StartIndex) { + const std::size_t NumParams = FD->getNumParams(); + assert(StartIndex < NumParams && "out of bounds for start"); + const ASTContext &Ctx = FD->getASTContext(); + + MixableParameterRange Ret; + // A parameter at index 'StartIndex' had been trivially "checked". + Ret.NumParamsChecked = 1; + + for (std::size_t I = StartIndex + 1; I < NumParams; ++I) { + const ParmVarDecl *Ith = FD->getParamDecl(I); + LLVM_DEBUG(llvm::dbgs() << "Check param #" << I << "...\n"); + + if (filter::isIgnoredParameter(Check, Ith)) { + LLVM_DEBUG(llvm::dbgs() << "Param #" << I << " is ignored. Break!\n"); + break; + } + + // Now try to go forward and build the range of [Start, ..., I, I + 1, ...] + // parameters that can be messed up at a call site. + decltype(Ret.Mixes) MixesOfIth; + for (std::size_t J = StartIndex; J < I; ++J) { + const ParmVarDecl *Jth = FD->getParamDecl(J); + LLVM_DEBUG(llvm::dbgs() + << "Check mix of #" << J << " against #" << I << "...\n"); + + Mix M{Jth, Ith, + calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx)}; + LLVM_DEBUG(llvm::dbgs() << "Mix flags (raw) : " + << formatBits(M.flags()) << '\n'); + M.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "Mix flags (after sanitise): " + << formatBits(M.flags()) << '\n'); + + assert(M.flags() != MIX_Invalid && "All flags decayed!"); + + if (M.flags() != MIX_None) + MixesOfIth.emplace_back(std::move(M)); + } + + if (MixesOfIth.empty()) { + // If there weren't any new mixes stored for Ith, the range is + // [Start, ..., I]. + LLVM_DEBUG(llvm::dbgs() + << "Param #" << I + << " does not mix with any in the current range. Break!\n"); + break; + } + + Ret.Mixes.insert(Ret.Mixes.end(), MixesOfIth.begin(), MixesOfIth.end()); + ++Ret.NumParamsChecked; // Otherwise a new param was iterated. + } + + return Ret; +} + +} // namespace model + +namespace filter { + +/// Returns whether the parameter's name or the parameter's type's name is +/// configured by the user to be ignored from analysis and diagnostic. +bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node) { + LLVM_DEBUG(llvm::dbgs() << "Checking if '" << Node->getName() + << "' is ignored.\n"); + + if (!Node->getIdentifier()) + return llvm::find(Check.IgnoredParameterNames, "\"\"") != + Check.IgnoredParameterNames.end(); + + StringRef NodeName = Node->getName(); + if (llvm::any_of( + Check.IgnoredParameterNames, + [NodeName](const std::string &E) { return NodeName == E; })) { + LLVM_DEBUG(llvm::dbgs() << "\tName ignored.\n"); + return true; + } + + std::string NodeTypeName = + Node->getType().getAsString(Node->getASTContext().getPrintingPolicy()); + StringRef CaptureName{NodeTypeName}; + LLVM_DEBUG(llvm::dbgs() << "\tType name is '" << CaptureName << "'\n"); + if (!NodeTypeName.empty()) { + if (llvm::any_of(Check.IgnoredParameterTypeSuffixes, + [CaptureName](const std::string &E) { + return !E.empty() && CaptureName.endswith(E); + })) { + LLVM_DEBUG(llvm::dbgs() << "\tType suffix ignored.\n"); + return true; + } + } + + return false; +} +} // namespace filter +} // namespace + +/// Matches functions that have at least the specified amount of parameters. +AST_MATCHER_P(FunctionDecl, parameterCountGE, unsigned, N) { + return Node.getNumParams() >= N; +} + +/// Matches *any* overloaded operator function. +AST_MATCHER(FunctionDecl, isOverloadedOperator) { + return Node.isOverloadedOperator(); +} + +/// The default value for the MinimumLength check option. +static constexpr unsigned DefaultMinimumLength = 2; + +/// Returns the DefaultMinimumLength if the Value of requested minimum length +/// is less than 2. Minimum lengths of 0 or 1 are not accepted. +static inline unsigned clampMinimumLength(const unsigned Value) { + return Value < 2 ? DefaultMinimumLength : Value; +} + +/// The default value for ignored parameter names. +static const std::string DefaultIgnoredParameterNames = serializeStringList( + {"\"\"", "iterator", "Iterator", "begin", "Begin", "end", "End", "first", + "First", "last", "Last", "lhs", "LHS", "rhs", "RHS"}); + +/// The default value for ignored parameter type suffixes. +static const std::string DefaultIgnoredParameterTypeSuffixes = + serializeStringList({"bool", "Bool", "_Bool", "it", "It", "iterator", + "Iterator", "inputit", "InputIt", "forwardit", + "FowardIt", "bidirit", "BidirIt", "constiterator", + "const_iterator", "Const_Iterator", "Constiterator", + "ConstIterator"}); + +/// Returns the diagnostic-friendly name of the node, or empty string. +static SmallString<64> getName(const NamedDecl *ND) { + SmallString<64> Name; + llvm::raw_svector_ostream OS{Name}; + ND->getNameForDiagnostic(OS, ND->getASTContext().getPrintingPolicy(), false); + return Name; +} + +/// Returns the diagnostic-friendly name of the node, or a constant value. +static SmallString<64> getNameOrUnnamed(const NamedDecl *ND) { + auto Name = getName(ND); + if (Name.empty()) + Name = ""; + return Name; +} + +EasilySwappableParametersCheck::EasilySwappableParametersCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + MinimumLength(clampMinimumLength( + Options.get("MinimumLength", DefaultMinimumLength))), + IgnoredParameterNames(parseStringList( + Options.get("IgnoredParameterNames", DefaultIgnoredParameterNames))), + IgnoredParameterTypeSuffixes( + parseStringList(Options.get("IgnoredParameterTypeSuffixes", + DefaultIgnoredParameterTypeSuffixes))) {} + +void EasilySwappableParametersCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "MinimumLength", MinimumLength); + Options.store(Opts, "IgnoredParameterNames", + serializeStringList(IgnoredParameterNames)); + Options.store(Opts, "IgnoredParameterTypeSuffixes", + serializeStringList(IgnoredParameterTypeSuffixes)); +} + +void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) { + const auto BaseConstraints = functionDecl( + // Only report for definition nodes, as fixing the issues reported + // requires the user to be able to change code. + isDefinition(), parameterCountGE(MinimumLength), + unless(isOverloadedOperator())); + + Finder->addMatcher( + functionDecl(BaseConstraints, + unless(ast_matchers::isTemplateInstantiation())) + .bind("func"), + this); + Finder->addMatcher( + functionDecl(BaseConstraints, isExplicitTemplateSpecialization()) + .bind("func"), + this); +} + +void EasilySwappableParametersCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *FD = Result.Nodes.getNodeAs("func"); + assert(FD); + + const PrintingPolicy &PP = FD->getASTContext().getPrintingPolicy(); + const std::size_t NumParams = FD->getNumParams(); + std::size_t MixableRangeStartIndex = 0; + + LLVM_DEBUG(llvm::dbgs() << "Begin analysis of " << getName(FD) << " with " + << NumParams << " parameters...\n"); + while (MixableRangeStartIndex < NumParams) { + if (filter::isIgnoredParameter(*this, + FD->getParamDecl(MixableRangeStartIndex))) { + LLVM_DEBUG(llvm::dbgs() + << "Parameter #" << MixableRangeStartIndex << " ignored.\n"); + ++MixableRangeStartIndex; + continue; + } + + const model::MixableParameterRange R = + model::modelMixingRange(*this, FD, MixableRangeStartIndex); + assert(R.NumParamsChecked > 0 && "Ensure forward progress!"); + MixableRangeStartIndex += R.NumParamsChecked; + if (R.NumParamsChecked < MinimumLength) { + LLVM_DEBUG(llvm::dbgs() << "Ignoring range of " << R.NumParamsChecked + << " lower than limit.\n"); + continue; + } + + const ParmVarDecl *First = R.getFirstParam(), *Last = R.getLastParam(); + std::string FirstParamTypeAsWritten = First->getType().getAsString(PP); + { + StringRef DiagText = "%0 adjacent parameters of %1 of similar type " + "('%2') are easily swapped by mistake"; + // TODO: This logic will get extended here with future flags. + + auto Diag = diag(First->getOuterLocStart(), DiagText) + << static_cast(R.NumParamsChecked) << FD; + + Diag << FirstParamTypeAsWritten; + } + + // Unfortunately, undersquiggly highlights are for some reason chewed up + // and not respected by diagnostics from Clang-Tidy. :( + diag(First->getLocation(), "the first parameter in the range is '%0'", + DiagnosticIDs::Note) + << getNameOrUnnamed(First); + diag(Last->getLocation(), "the last parameter in the range is '%0'", + DiagnosticIDs::Note) + << getNameOrUnnamed(Last); + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -121,6 +121,13 @@ Finds structs that are inefficiently packed or aligned, and recommends packing and/or aligning of said structs as needed. +- New :doc:`bugprone-easily-swappable-parameters + ` check. + + Finds function definitions where parameters of convertible types follow each + other directly, making call sites prone to calling the function with + swapped (or badly ordered) arguments. + - New :doc:`cppcoreguidelines-prefer-member-initializer ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst @@ -0,0 +1,88 @@ +.. title:: clang-tidy - bugprone-easily-swappable-parameters + +bugprone-easily-swappable-parameters +==================================== + +Finds function definitions where parameters of convertible types follow each +other directly, making call sites prone to calling the function with +swapped (or badly ordered) arguments. + +.. code-block:: c++ + + void drawPoint(int X, int Y) { /* ... */ } + FILE *open(const char *Dir, const char *Name, Flags Mode) { /* ... */ } + +A potential call like ``drawPoint(-2, 5)`` or ``openPath("a.txt", "tmp", Read)`` +is perfectly legal from the language's perspective, but might not be what the +developer of the function intended. + +More elaborate and type-safe constructs, such as opaque typedefs or strong +types should be used instead, to prevent a mistaken order of arguments. + +.. code-block:: c++ + + struct Coord2D { int X; int Y; }; + void drawPoint(const Coord2D Pos) { /* ... */ } + + FILE *open(const Path &Dir, const Filename &Name, Flags Mode) { /* ... */ } + +Due to the potentially elaborate refactoring and API-breaking that is necessary +to strengthen the type safety of a project, no automatic fix-its are offered. + +Options +------- + +Filtering options +^^^^^^^^^^^^^^^^^ + +Filtering options can be used to lessen the size of the diagnostics emitted by +the checker, whether the aim is to ignore certain constructs or dampen the +noisiness. + +.. option:: MinimumLength + + The minimum length required from an adjacent parameter sequence to be + diagnosed. + Defaults to ``2``. + Might be any positive integer. + For example, in the case of ``3``, the above examples will not be matched. + If ``0`` or ``1`` is given, the default value ``2`` will be used instead. + +.. option:: IgnoredParameterNames + + The list of parameter **names** that should never be considered part of a + swappable adjacent parameter sequence. + The value is a ``;``-separated list of names. + To ignore unnamed parameters, add ``""`` to the list verbatim (not the + empty string, but the two quotes, potentially escaped!). + **This options is case-sensitive!** + + By default, the following parameter names, and their Uppercase-initial + variants are ignored: + ``""`` (unnamed parameters), ``iterator``, ``begin``, ``end``, ``first``, + ``last``, ``lhs``, ``rhs``. + +.. option:: IgnoredParameterTypeSuffixes + + The list of parameter **type names** that should never be considered part of + a swappable adjacent parameter sequence. + Parameters which type, as written in the source code, end with an element + of this option will be ignored. + The value is a ``;``-separated list of names. + **This option is case-sensitive!** + + By default, the following, and their lowercase-initial variants are ignored: + ``bool``, ``It``, ``Iterator``, ``InputIt``, ``ForwardIt``, ``BidirIt``, + ``Const_Iterator``, ``ConstIterator``. + In addition, ``_Bool`` (but not ``_bool``) is also part of the default + value. + + +Limitations +----------- + +This check does not investigate functions that were generated by the compiler +through function template instantiation, as the user has no chance of "fixing" +the reported issue. +As such, in the case of function templates, only primary template definitions +and explicit specialisations are attempted to be matched. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -57,6 +57,7 @@ `bugprone-copy-constructor-init `_, "Yes" `bugprone-dangling-handle `_, `bugprone-dynamic-static-initializers `_, + `bugprone-easily-swappable-parameters `_, `bugprone-exception-escape `_, `bugprone-fold-init-type `_, `bugprone-forward-declaration-namespace `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp @@ -0,0 +1,33 @@ +// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: "\"\";Foo;Bar"}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"} \ +// RUN: ]}' -- + +void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed. + +void nothingIgnored(int I, int J) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'nothingIgnored' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'J' + +void ignoredParameter(int Foo, int I, int J) {} +// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 2 adjacent parameters of 'ignoredParameter' of similar type ('int') +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'J' + +void ignoredParameterBoth(int Foo, int Bar) {} // NO-WARN. + +struct S {}; +struct T {}; +struct MyT {}; + +void notIgnoredType(S S1, S S2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'notIgnoredType' of similar type ('S') +// CHECK-MESSAGES: :[[@LINE-2]]:23: note: the first parameter in the range is 'S1' +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'S2' + +void ignoredTypeExact(T T1, T T2) {} // NO-WARN. + +void ignoredTypeSuffix(MyT M1, MyT M2) {} // NO-WARN. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp @@ -0,0 +1,141 @@ +// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \ +// RUN: ]}' -- + +#define assert(X) + +void declaration(int Param, int Other); // NO-WARN: No chance to change this function. + +struct S {}; + +S *allocate() { return nullptr; } // NO-WARN: 1 parameter. +void allocate(S **Out) {} // NO-WARN: 1 parameter. +bool operator<(const S &LHS, const S &RHS) { return true; } // NO-WARN: Operator. + +void redeclChain(int, int, int); +void redeclChain(int I, int, int); +void redeclChain(int, int J, int); +void redeclChain(int I, int J, int K) {} +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:36: note: the last parameter in the range is 'K' + +void copyMany(S *Src, S *Dst, unsigned Num) {} +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'copyMany' of similar type ('S *') +// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'Src' +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: the last parameter in the range is 'Dst' + +template +bool binaryPredicate(T L, U R) { return false; } // NO-WARN: Distinct types in template. + +template <> // Explicit specialisation. +bool binaryPredicate(S *L, S *R) { return true; } +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent parameters of 'binaryPredicate' of similar type ('S *') +// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in the range is 'L' +// CHECK-MESSAGES: :[[@LINE-3]]:31: note: the last parameter in the range is 'R' + +template +T algebraicOperation(T L, T R) { return L; } +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent parameters of 'algebraicOperation' of similar type ('T') +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'L' +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'R' + +void applyBinaryToS(S SInstance) { // NO-WARN: 1 parameter. + assert(binaryPredicate(SInstance, SInstance) != + binaryPredicate(&SInstance, &SInstance)); + // NO-WARN: binaryPredicate(S, S) is instantiated, but it's not written + // by the user. +} + +void unnamedParameter(int I, int, int K, int) {} +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 4 adjacent parameters of 'unnamedParameter' of similar type ('int') +// CHECK-MESSAGES: :[[@LINE-2]]:27: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:45: note: the last parameter in the range is '' + +void multipleDistinctTypes(int I, int J, long L, long M) {} +// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 2 adjacent parameters of 'multipleDistinctTypes' of similar type ('int') +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:39: note: the last parameter in the range is 'J' +// CHECK-MESSAGES: :[[@LINE-4]]:42: warning: 2 adjacent parameters of 'multipleDistinctTypes' of similar type ('long') +// CHECK-MESSAGES: :[[@LINE-5]]:47: note: the first parameter in the range is 'L' +// CHECK-MESSAGES: :[[@LINE-6]]:55: note: the last parameter in the range is 'M' + +void variableAndPtr(int I, int *IP) {} // NO-WARN: Not the same type. + +void differentPtrs(int *IP, long *LP) {} // NO-WARN: Not the same type. + +typedef int MyInt1; +using MyInt2 = int; + +void typedefAndTypedef1(MyInt1 I1, MyInt1 I2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'typedefAndTypedef1' of similar type ('MyInt1') +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'I1' +// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'I2' + +void typedefAndTypedef2(MyInt2 I1, MyInt2 I2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'typedefAndTypedef2' of similar type ('MyInt2') +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'I1' +// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'I2' + +void throughTypedef(int I, MyInt1 J) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'throughTypedef' of similar type ('int') +// CHECK-MESSAGES: :[[@LINE-2]]:25: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in the range is 'J' + +void betweenTypedef(MyInt1 I, MyInt2 J) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'betweenTypedef' of similar type ('MyInt1') +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in the range is 'J' + +typedef long MyLong1; +using MyLong2 = long; + +void throughTypedefToOtherType(MyInt1 I, MyLong1 J) {} // NO-WARN: Not the same type. + +void qualified1(int I, const int CI) {} // NO-WARN: Not the same type. + +void qualified2(int I, volatile int VI) {} // NO-WARN: Not the same type. + +void qualified3(int *IP, const int *CIP) {} // NO-WARN: Not the same type. + +void qualified4(const int CI, const long CL) {} // NO-WARN: Not the same type. + +using CInt = const int; + +void qualifiedThroughTypedef1(int I, CInt CI) {} // NO-WARN: Not the same type. + +void qualifiedThroughTypedef2(CInt CI1, const int CI2) {} // NO-WARN: Not the same type. +// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 2 adjacent parameters of 'qualifiedThroughTypedef2' of similar type ('CInt') +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'CI1' +// CHECK-MESSAGES: :[[@LINE-3]]:51: note: the last parameter in the range is 'CI2' + +void reference1(int I, int &IR) {} // NO-WARN: Not the same type. + +void reference2(int I, const int &CIR) {} // NO-WARN: Not the same type. + +void reference3(int I, int &&IRR) {} // NO-WARN: Not the same type. + +void reference4(int I, const int &&CIRR) {} // NO-WARN: Not the same type. + +template +struct Pair {}; + +void templateParam1(Pair P1, Pair P2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'templateParam1' of similar type ('Pair') +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: the first parameter in the range is 'P1' +// CHECK-MESSAGES: :[[@LINE-3]]:55: note: the last parameter in the range is 'P2' + +void templateParam2(Pair P1, Pair P2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 2 adjacent parameters of 'templateParam2' of similar type ('Pair') +// CHECK-MESSAGES: :[[@LINE-2]]:37: note: the first parameter in the range is 'P1' +// CHECK-MESSAGES: :[[@LINE-3]]:57: note: the last parameter in the range is 'P2' + +void templateParam3(Pair P1, Pair P2) {} // NO-WARN: Not the same type. + +template +struct Coord {}; + +void templateAndOtherTemplate1(Pair P, Coord C) {} // NO-WARN: Not the same type. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp @@ -0,0 +1,24 @@ +// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \ +// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \ +// RUN: ]}' -- + +int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters. + +int magic(int Left, int Right, int X, int Y) { return 0; } +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters of 'magic' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters] +// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in the range is 'Left' +// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'Y' + +void multipleDistinctTypes(int I, int J, int K, + long L, long M, + double D, double E, double F) {} +// CHECK-MESSAGES: :[[@LINE-3]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('int') +// CHECK-MESSAGES: :[[@LINE-4]]:32: note: the first parameter in the range is 'I' +// CHECK-MESSAGES: :[[@LINE-5]]:46: note: the last parameter in the range is 'K' +// NO-WARN: The [long, long] range is length of 2. +// CHECK-MESSAGES: :[[@LINE-5]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('double') +// CHECK-MESSAGES: :[[@LINE-6]]:35: note: the first parameter in the range is 'D' +// CHECK-MESSAGES: :[[@LINE-7]]:55: note: the last parameter in the range is 'F'