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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
