[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 18 08:54:03 PDT 2021
aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:377
+
+ bool indicatesMixability() const { return Flags > MIX_None; }
+
----------------
Should this be `MIX_WorkaroundDisableCanonicalEquivalence` instead of `MIX_None`?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:486-487
+static MixData
+approximateImplicitConversion(const TheCheck &Check, const QualType LType,
+ const QualType RType, const ASTContext &Ctx,
+ ImplicitConversionModellingMode ImplicitMode);
----------------
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:500-501
+static MixData
+calculateMixability(const TheCheck &Check, const QualType LType,
+ const QualType RType, const ASTContext &Ctx,
+ ImplicitConversionModellingMode ImplicitMode) {
----------------
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:520
// user to understand.
- if (isa<ParenType>(LType.getTypePtr())) {
- LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is ParenType.\n");
+ if (isa<ParenType, ElaboratedType>(LType.getTypePtr())) {
+ LLVM_DEBUG(llvm::dbgs()
----------------
What about a `DecayedType`?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+ // Certain kinds unfortunately need to be side-stepped for canonical type
+ // matching.
+ if (LType->getAs<FunctionProtoType>() || RType->getAs<FunctionProtoType>()) {
+ // Unfortunately, the canonical type of a function pointer becomes the
+ // same even if exactly one is "noexcept" and the other isn't, making us
+ // give a false positive report irrespective of implicit conversions.
+ LLVM_DEBUG(llvm::dbgs()
----------------
whisperity wrote:
> @aaron.ballman Getting ahead of the curve here. I understand this is ugly. Unfortunately, no matter how much I read the standard, I don't get anything of "canonical type" mentioned, it seems to me this concept is something inherent to the model of Clang.
>
> Basically why this is here: imagine a `void (*)() noexcept` and a `void (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a //one-way// implicit conversion from the `noexcept` to the non-noexcept ("noexceptness can be discarded", similarly to how "constness can be gained")
> Unfortunately, because this is a one-way implicit conversion, it won't return on the code path earlier for implicit conversions.
>
> Now because of this, the recursive descent in our code will reach the point when the two innermost `FunctionProtoType`s are in our hands. As a fallback case, we simply ask Clang "Hey, do //you// think these two are the same?". And for some weird reason, Clang will say "Yes."... While one of them is a `noexcept` function and the other one isn't.
>
> I do not know the innards of the AST well enough to even have a glimpse of whether or not this is a bug. So that's the reason why I went ahead and implemented this side-stepping logic. Basically, as the comment says, it'lll **force** the information of "No matter what you do, do NOT consider these mixable!" back up the recursion chain, and handle it appropriately later.
> Unfortunately, no matter how much I read the standard, I don't get anything of "canonical type" mentioned, it seems to me this concept is something inherent to the model of Clang.
It is more of a Clang-centric concept. Basically, a canonical type is one that's had all of the typedefs stripped off it.
> Now because of this, the recursive descent in our code will reach the point when the two innermost FunctionProtoTypes are in our hands. As a fallback case, we simply ask Clang "Hey, do you think these two are the same?". And for some weird reason, Clang will say "Yes."... While one of them is a noexcept function and the other one isn't.
I think a confounding factor in this area is that `noexcept` did not used to be part of the function type until one day it started being a part of the function type. So my guess is that this is fallout from this sort of thing: https://godbolt.org/z/EYfj8z (which may be worth keeping in mind when working on the check).
================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:758
+approximateStandardConversionSequence(const TheCheck &Check,
+ const QualType From, const QualType To,
+ const ASTContext &Ctx) {
----------------
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75041/new/
https://reviews.llvm.org/D75041
More information about the cfe-commits
mailing list