[libcxx-commits] [PATCH] D111283: [clang] template / auto deduction deduces common sugar

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov marked 2 inline comments as done.
mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12225-12227
+#define NON_CANONICAL_TYPE(Class, Base) UNEXPECTED_TYPE(Class, "non-canonical")
+#define TYPE(Class, Base)
+#include "clang/AST/TypeNodes.inc"
----------------
rsmith wrote:
> What guarantees that we won't see never-canonical types here?
> 
> I understand why we won't see sugar-free types (we can't have two `Type*`s that are different but represent the same type if they're sugar-free), and why we won't see non-unique types here (we can't have two `Type*`s that are different but represent the same type if they're not uniqued). But why can't we find, say, two different `TypedefType`s here that desugar to the same type?
The idea is that this function is only called from a context where removeDifferentTopLevelSugar / unwrapSugar will have removed all top-level nodes which are sugar. So in particular, we will never see here any type nodes which have an implementation for is isSugared which always returns true, such as the TypedefType case.

So this is peculiar to this initial implementation, which never tries to "merge" sugar nodes.
There is value in trying to merge those, but you can imagine that the implementation for that is much more complicated and needs more thinking to get right and efficient.

For example for the TypedefType case right now:

If we had two different typedefs with the same underlying type, then there seems to be no sensible choice here except stripping off the typedef, I mean what Decl would we put on the TypedefType? Right now nullptr would not work since we always take the underlying type from the Decl, but even if it did, the value of having a nullptr decl seems questionable given the amount of trouble this could cause in other code.

If the TypedefTypes pointed to the same TypedefDecl, then it would make sense to create a new one with the "common" Decl between them, such as the earliest declaration or the canonical one, but might not be worth the extra complexity in the overall logic just for this effect.

Note that on my D127695, I implemented resugared TypedefTypes, which can have a different (but canonically the same) underlying type. With that, we would have more to do here and I think then it would be worth improving this further.

Otherwise, there is other interesting sugar we could try merging in the future as well, such as ElaboratedType and alias TemplateSpecializationTypes.


================
Comment at: clang/lib/AST/ASTContext.cpp:12253-12254
+    const auto *AX = cast<AutoType>(X), *AY = cast<AutoType>(Y);
+    assert(AX->getDeducedType().isNull());
+    assert(AY->getDeducedType().isNull());
+    assert(AX->getKeyword() == AY->getKeyword());
----------------
rsmith wrote:
> I don't understand why the deduced type must be null here. I think it usually will be, because if the deduced type were non-null, it would have to be identical (because we know desugaring one more level results in identical types), and in that case we'd typically have produced the same `AutoType`. But it seems like we could have two constrained `AutoType`s here that desugar to the same type but differ in the spelling of their constraints, in which case the common type should presumably have a deduced type. I wonder if we should just be checking `AX->getDeducedType() == AY->getDeducedType()` here and passing in `AX->getDeducedType()` as the deduced type below?
Well if they desugared to anything, then they would never show up in this function, they would have been stripped off by unwrapSugar as I explained in the previous comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111283/new/

https://reviews.llvm.org/D111283