whisperity updated this revision to Diff 318220.
whisperity retitled this revision from "[clang-tidy] Add
'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type'
check" to "[clang-tidy] Add 'bugprone-easily-swappable-parameters' check".
whisperity edited the summary of this revision.
whisperity removed a reviewer: baloghadamsoftware.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity removed a subscriber: o.gyorgy.
whisperity added a comment.
Refactored the check and rebased it against "current" `master`. This version
now has the tests rewritten and fixed. In addition, this patch only introduces
the **very basic** frame of the check (i.e. only //strict type equality//),
nothing more. This is so that review can be done in a more digestible way.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69560/new/
https://reviews.llvm.org/D69560
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===================================================================
--- /dev/null
+++ 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'
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -0,0 +1,112 @@
+// 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 <typename T, typename U>
+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<S *, S *>' 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 <typename T>
+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 '<unnamed>'
+
+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) {} // NO-WARN: Not the same type.
+
+void betweenTypedef(MyInt1 I, MyInt2 J) {} // NO-WARN: Not the same type.
+
+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.
+
+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.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
===================================================================
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -57,6 +57,7 @@
`bugprone-copy-constructor-init <bugprone-copy-constructor-init.html>`_, "Yes"
`bugprone-dangling-handle <bugprone-dangling-handle.html>`_,
`bugprone-dynamic-static-initializers <bugprone-dynamic-static-initializers.html>`_,
+ `bugprone-easily-swappable-parameters <bugprone-easily-swappable-parameters.html>`_,
`bugprone-exception-escape <bugprone-exception-escape.html>`_,
`bugprone-fold-init-type <bugprone-fold-init-type.html>`_,
`bugprone-forward-declaration-namespace <bugprone-forward-declaration-namespace.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
===================================================================
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
+ <clang-tidy/checks/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
<clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
===================================================================
--- /dev/null
+++ 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<std::string> IgnoredParameterNames;
+
+ /// The parameter typename suffixes (as written in the source code) to be
+ /// ignored.
+ const std::vector<std::string> IgnoredParameterTypeSuffixes;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EASILYSWAPPABLEPARAMETERSCHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -0,0 +1,403 @@
+//===--- 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 <std::size_t Width>
+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 <typename T> static inline std::string formatBits(T &&V) {
+ return formatBitsImpl<sizeof(T) * 8>(V);
+}
+#else
+
+template <typename T> 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 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 parameters mix trivially, and are the same type.
+
+#undef FB
+
+ LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue =*/MIX_None)
+};
+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<Mix>::value &&
+ std::is_trivially_move_constructible<Mix>::value &&
+ std::is_trivially_move_assignable<Mix>::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<Mix, 8> 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.
+
+ 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 = "<unnamed>";
+ 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<FunctionDecl>("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<unsigned>(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
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ 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<DynamicStaticInitializersCheck>(
"bugprone-dynamic-static-initializers");
+ CheckFactories.registerCheck<EasilySwappableParametersCheck>(
+ "bugprone-easily-swappable-parameters");
CheckFactories.registerCheck<ExceptionEscapeCheck>(
"bugprone-exception-escape");
CheckFactories.registerCheck<FoldInitTypeCheck>(
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
