mizvekov added a comment.
In D111283#3716104 <https://reviews.llvm.org/D111283#3716104>, @davrec wrote:
> The first paragraph says X and Y must have the same canonical type, the
> second suggests they need not.
The first paragraph is talking about canonical equality, or if you take it that
types 'refer' to their canonical types, referential equality.
Ie 'X.getCanonicalType() == Y.getCanonicalType()'
For short, when there is referential equality, I will say that the types are
'the same'. This matches terminology in Clang, for example see the helper
ASTContext::hasSameType.
When there is structural equality, ie 'X == Y', I will say for short the types
are 'identical'.
The second paragraph is talking about 'Canonical nodes', not 'Canonical types'.
A canonical node is a type node for which 'isSugared' method returns false.
This is not the same thing as saying that a Canonical node is a canonical type.
For example, 'PointerType' nodes are always canonical nodes, because it has an
'isSugared' implementation which always returns false:
bool isSugared() const { return false; }
But pointer types can be non-canonical, in particular when the PointeeType is
not a canonical type.
In D111283#3716104 <https://reviews.llvm.org/D111283#3716104>, @davrec wrote:
> In all the test cases, they have the same canonical type.
Well not exactly, we also handle and test the case they are also the same when
'unqualified'.
The case here is function types that differ only in top-level qualifiers of
parameters, so this is used only when we are recursing down a FunctionType.
Those FunctionTypes should be treated as the same, ie you can't overload based
on differences in top-level qualifiers of function parameters.
But this should not be treated as a semantic adjustment, like we do for decays,
eg in array parameters, because we still need to consider those qualifiers on
the parameter itself within the function definition, unlike what happens with
decays (where we basically just rewrite the type of the parameter).
See the `t8` and `t11` test cases in `clang/test/SemaCXX/sugared-auto.cpp`,
which test top-level differences in const and ObjectiveC lifetime qualifiers
respectively.
> IIUC the second paragraph only applies to the stuff done in D130308
> <https://reviews.llvm.org/D130308>; is that correct?
No, it's talking about the stuff in this patch.
In this patch, we unify Canonical nodes that differ, but don't unify different
non-canonical nodes (ie those which 'isSugared()' returns true), that last part
is done in D130308 <https://reviews.llvm.org/D130308>.
> If so, the description should clearly state this patch only handles the case
> when the canonical types of X and Y are identical, and note that if the
> canonical types of X and Y differ, this patch does nothing, but D130308
> <https://reviews.llvm.org/D130308> adds some additional logic to search for
> common sugar in child type nodes of X and Y to use in the merged type.
Well it's not that we do nothing if the types are not the same!
That the inputs should be the same (but not necessaritly identical) is an
invariant, ie a contract to use this function.
In a debug build, we will check that this invariant is respected, asserting
otherwise.
> And of course please add a full description to D130308
> <https://reviews.llvm.org/D130308> as well when you have a chance.
Sure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111283/new/
https://reviews.llvm.org/D111283
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
