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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122
 
 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
 
----------------
martong wrote:
> FB stands for FunnyBitmask? Could you please either describe that in a comment or just spell it out?
FlagBit. @alexfh suggested in the base check to use hexa literals. I'm not too sold about that, but maybe we can cut the macro out and keep the bit shift instructions in. Given that the check has more or less earned its final form (for now), we know how many bits we need!


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:182
+  /// The intermediate type after the first Standard Conversion Sequence.
+  QualType AfterPreStd;
+
----------------
martong wrote:
> Maybe it's just me, but AfterPre sounds ambiguous and AfterPost seems redundant.
Should I use "AfterFirstStandard" or "AfterFirstStd"?

AfterPost isn't redundant. An implicit conversion sequence might be of **3 steps**. Consider in a hypothetical `void f(Complex x, const double d)`.

`Complex` ---(PRE: std qual adjustment)--> `const Complex` ---(UD: user defined conv)--> `double` ---(POST: std qual adjustment)--> `const double`

And because in case of many conversions a lot of combinations need to be checked, we can't have `AfterPost` be the same as `End`. First, because of the combinations, second, because of the //other// things the check is doing.

`void g(ComplexTypedef CT, ConstDoubleTypedef CDT);`

In this case, `Start` and `End` are the typedefs, and the inner sequence is the same as before. And in order to generate the diagnostic, we also need to have **both** pieces of information.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:210
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector<QualType, 4> getInvolvedTypesInSequence() const {
+    SmallVector<QualType, 4> Ret;
----------------
martong wrote:
> So, we can never return with a vector with size > 4?
`N` in `SmallVector<T, N>` only specifies the pre-allocated smallness size. AFAIK, if you have >N elements, it will just put the vector out onto the heap.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:42
 
-  /// Whether to consider an unqualified and a qualified type mixable.
+  /// Whether to consider differently qualified versions of the same type
+  /// mixable.
----------------
martong wrote:
> "qualified"
> Do you consider `volatile` here as well, or just `const`?
Const, volatile, and in C mode, restrict, too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75041/new/

https://reviews.llvm.org/D75041