martong added inline comments.
================ 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() ---------------- aaron.ballman wrote: > 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). About `noexcept`: we've faced a similar problem in the `ASTImporter` library. We could not import a noexcept function's definition if we already had one without the noexcept specifier. Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function types based on their noexcept specifier (and we even check the noexcept expression).: ``` TEST_F(StructuralEquivalenceFunctionTest, Noexcept) { auto t = makeNamedDecls("void foo();", "void foo() noexcept;", Lang_CXX11); EXPECT_FALSE(testStructuralMatch(t)); } TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) { auto t = makeNamedDecls("void foo() noexcept(false);", "void foo() noexcept(true);", Lang_CXX11); EXPECT_FALSE(testStructuralMatch(t)); } ``` May be in these special cases it would worth to reuse the ASTStructuralEquivalenceContext class? 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