whisperity 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()
----------------
whisperity wrote:
> martong wrote:
> > 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?
> Definitely, good catch, @martong, thank you very much! @aaron.ballman, what
> do you think? If I see this right, `StructuralEquivalenceContext` is part of
> `libClangAST` so should be readily available.
>
> The only issue I'm seeing is that this class takes non-**`const`**
> `ASTContext` and `Decl` nodes...
Well, I started looking into putting up `const` whereever possible into the
aforementioned type, but I hit a hurdle. When checking equivalence of records,
the algorithm tries to ASTImport-complete the definition of a record if it's
not fully defined yet... which is **not** a `const` method... I'll look into it
some more, maybe we can do an overload on whether you're Structual equivalence
checking in a const context or a non-const context.
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
