[PATCH] D135088: [Clang] make canonical AutoType constraints-free

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:5736-5737
   llvm::FoldingSetNodeID ID;
   AutoType::Profile(ID, *this, DeducedType, Keyword, IsDependent,
                     TypeConstraintConcept, TypeConstraintArgs);
   if (AutoType *AT = AutoTypes.FindNodeOrInsertPos(ID, InsertPos))
----------------
mizvekov wrote:
> FYI (not asking you to fix this on this patch) there is a pre-existing bug here where we are not profiling the node on the `isPack` flag.
> FYI (not asking you to fix this on this patch) there is a pre-existing bug here where we are not profiling the node on the `isPack` flag.

Thanks. Great to know.


================
Comment at: clang/lib/AST/ASTContext.cpp:5745-5746
+      if (TypeConstraintConcept) {
+        Canon = getAutoTypeInternal(QualType(), Keyword, false, IsPack, nullptr,
+                                    {}, true);
         // Find the insert position again.
----------------
mizvekov wrote:
> Also adds back the `isDependent` flag. Any reason to have removed it?
I was thinking the replacement type is null, so there is no need to depend on instantiation?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8664-8668
+  if (QualType RT = FD->getDeclaredReturnType();
+      DCK == DefaultedComparisonKind::ThreeWay &&
+      RT->getContainedDeducedType() &&
+      (!Context.hasSameType(RT, Context.getAutoDeductType()) ||
+       RT->getContainedAutoType()->isConstrained())) {
----------------
mizvekov wrote:
> I think this might crash if the three-way comparison is defaulted with a deduced template specialization.
> 
> Something like:
> ```
> template <class> struct A {};
> 
> struct P {
>   friend A operator<=>(const P&, const P&) = default;
> };
> ```
> 
> The problem is that `RT->getContainedDeducedType()` will return non-null, because DTSTs are also deduced types, but `RT->getContainedAutoType()` will return null, because DTSTs are not AutoTypes.
> I think this might crash if the three-way comparison is defaulted with a deduced template specialization.
> The problem is that `RT->getContainedDeducedType()` will return non-null, because DTSTs are also deduced types, but `RT->getContainedAutoType()` will return null, because DTSTs are not AutoTypes.

`RT->getContainedAutoType()` will be called only when `RT` is `AutoType`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135088