Commit e33d0478 authored by Whisperity's avatar Whisperity
Browse files

[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability... 

[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

Adds a relaxation option ModelImplicitConversions which will make the check
report for cases where parameters refer to types that are implicitly
convertible to one another.

Example:

    struct IntBox { IntBox(int); operator int(); };
    void foo(int i, double d, IntBox ib) {}

Implicit conversions are the last to model in the set of things that are
reasons for the possibility of a function being called the wrong way which is
not always immediately apparent when looking at the function (signature or
call).

Reviewed By: aaron.ballman, martong

Differential Revision: http://reviews.llvm.org/D75041
parent 961e9e6a

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

+1065 −93

File changed.

Preview size limit exceeded, changes collapsed.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

+7 −1
Original line number Diff line number Diff line
@@ -39,8 +39,14 @@ public: 
  /// ignored.

  const std::vector<std::string> IgnoredParameterTypeSuffixes;



  /// Whether to consider an unqualified and a qualified type mixable.

  /// Whether to consider differently qualified versions of the same type

  /// mixable.

  const bool QualifiersMix;



  /// Whether to model implicit conversions "in full" (conditions apply)

  /// during analysis and consider types that are implicitly convertible to

  /// one another mixable.

  const bool ModelImplicitConversions;

};



} // namespace bugprone

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst

+66 −5
Original line number Diff line number Diff line
@@ -57,8 +57,40 @@ report longer mixable ranges. 


    .. code-block:: c++



        void *memcpy(const void *Destination, void *Source, std::size_t N) {}

        void *memcpy(const void *Destination, void *Source, std::size_t N) { /* ... */ }



.. option:: ModelImplicitConversions



    Whether to consider parameters of type ``T`` and ``U`` mixable if there

    exists an implicit conversion from ``T`` to ``U`` and ``U`` to ``T``.

    If `false`, the check will not consider implicitly convertible types for

    mixability.

    `True` turns warnings for implicit conversions on.

    Defaults to `true`.



    The following examples produce a diagnostic only if

    `ModelImplicitConversions` is enabled:



    .. code-block:: c++



        void fun(int Int, double Double) { /* ... */ }

        void compare(const char *CharBuf, std::string String) { /* ... */ }



    .. note::



        Changing the qualifiers of an expression's type (e.g. from ``int`` to

        ``const int``) is defined as an *implicit conversion* in the C++

        Standard.

        However, the check separates this decision-making on the mixability of

        differently qualified types based on whether `QualifiersMix` was

        enabled.



        For example, the following code snippet will only produce a diagnostic

        if **both** `QualifiersMix` and `ModelImplicitConversions` are enabled:



        .. code-block:: c++



            void fun2(int Int, const double Double) { /* ... */ }



Filtering options

^^^^^^^^^^^^^^^^^
@@ -133,7 +165,7 @@ None of the following cases produce a diagnostic: 
    template <typename T, typename U>

    int add(T X, U Y) { return X + Y };



    void TheseAreNotWarnedAbout() {

    void theseAreNotWarnedAbout() {

        printf("%d %d\n", 1, 2);   // Two ints passed, they could be swapped.

        someOldCFunction(1, 2, 3); // Similarly, multiple ints passed.
@@ -153,14 +185,43 @@ not diagnosed. 


    // Diagnosed: Explicit instantiation was done by the user, we can prove it

    // is the same type.

    void Explicit(int A, Vector<int>::element_type B) { /* ... */ }

    void instantiated(int A, Vector<int>::element_type B) { /* ... */ }



    // Diagnosed: The two parameter types are exactly the same.

    template <typename T>

    void Exact(typename Vector<T>::element_type A,

    void exact(typename Vector<T>::element_type A,

               typename Vector<T>::element_type B) { /* ... */ }



    // Skipped: The two parameters are both 'T' but we can not prove this

    // without actually instantiating.

    template <typename T>

    void FalseNegative(T A, typename Vector<T>::element_type B) { /* ... */ }
 
    void falseNegative(T A, typename Vector<T>::element_type B) { /* ... */ }



In the context of *implicit conversions* (when `ModelImplicitConversions` is

enabled), the modelling performed by the check

warns if the parameters are swappable and the swapped order matches implicit

conversions.

It does not model whether there exists an unrelated third type from which

*both* parameters can be given in a function call.

This means that in the following example, even while ``strs()`` clearly carries

the possibility to be called with swapped arguments (as long as the arguments

are string literals), will not be warned about.



.. code-block:: c++



    struct String {

        String(const char *Buf);

    };



    struct StringView {

        StringView(const char *Buf);

        operator const char *() const;

    };



    // Skipped: Directly swapping expressions of the two type cannot mix.

    // (Note: StringView -> const char * -> String would be **two**

    // user-defined conversions, which is disallowed by the language.)

    void strs(String Str, StringView SV) { /* ... */ }



    // Diagnosed: StringView implicitly converts to and from a buffer.

    void cStr(StringView SV, const char *Buf() { /* ... */ }

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp

+2 −1
Original line number Diff line number Diff line
@@ -3,7 +3,8 @@ 
// 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:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \

// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \

// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \

// RUN:  ]}' --



void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp

0 → 100644
+15 −0
Original line number Diff line number Diff line 
// 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:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \

// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \

// RUN:  ]}' --



void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); }

// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'numericAndQualifierConversion' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]

// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'I'

// CHECK-MESSAGES: :[[@LINE-3]]:56: note: the last parameter in the range is 'CD'

// CHECK-MESSAGES: :[[@LINE-4]]:43: note: 'int' and 'const double' parameters accept and bind the same kind of values

// CHECK-MESSAGES: :[[@LINE-5]]:43: note: 'int' and 'const double' may be implicitly converted: 'int' -> 'const double' (as 'double'), 'const double' (as 'double') -> 'int'