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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122
 
 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
 
----------------
whisperity wrote:
> 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!
FlagBit, I like it, perhaps we should rename `FB` to `FlagBit`?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:182
+  /// The intermediate type after the first Standard Conversion Sequence.
+  QualType AfterPreStd;
+
----------------
whisperity wrote:
> 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.
Thanks for this explanation, it makes clearer! However, this highlights that a way better description (in the form of comments) is needed for these `QualType` members. Especially this line could really increase the readability.
```
Complex ---(PRE: std qual adjustment)--> const Complex ---(UD: user defined conv)--> double ---(POST: std qual adjustment)--> const double
```

> 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.

I think it would be useful to document the cases with examples when `End` is not equal to `AfterPost` and such.


================
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;
----------------
whisperity wrote:
> 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.
I meant the number 4, not SmallVector. So, is there a case when the conversion sequence is longer than 4? If it can be longer, then where do you store the types, you have 4 `QualType` members if I am not wrong. (Also, I am not an expert of conversions.) 

To be honest, it is so terrible that we cannot reuse [[ https://clang.llvm.org/doxygen/classclang_1_1StandardConversionSequence.html | clang::StandardConversionSequence ]]


================
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.
----------------
whisperity wrote:
> martong wrote:
> > "qualified"
> > Do you consider `volatile` here as well, or just `const`?
> Const, volatile, and in C mode, restrict, too.
Great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041