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