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

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:11551-11552
+    if (Unqualified && !Ctx.hasSameType(X, Y)) {
+      assert(Ctx.hasSameUnqualifiedType(X, Y));
+      auto XQuals = X.getCVRQualifiers(), YQuals = Y.getCVRQualifiers();
+      X.addFastQualifiers(YQuals & ~XQuals);
----------------
rsmith wrote:
> What should happen if there's a qualifier difference other than a CVR qualifier? Eg, address space or ObjC lifetime.
I have updated the `ASContext::getCommonSugaredType` to accept an `Unqualified` parameter to handle this case.
We will strip qualifiers from each other until the types are the same.


================
Comment at: clang/lib/AST/ASTContext.cpp:11578
+  default:
+    return X;
+  }
----------------
rsmith wrote:
> For `TemplateArgument::ArgKind::Declaration`, I think it'd make sense to take the common type of the `getParamTypeForDecl`s.
Yeah, but this has been dead code since we went back to canonical template parameter / arguments.
I have removed it and put some unreachables in place.
We will circle back around to add it with this suggestion later.


================
Comment at: clang/lib/AST/ASTContext.cpp:11790-11801
+    if (EPIX.ExceptionSpec.Exceptions.size() ==
+        EPIY.ExceptionSpec.Exceptions.size()) {
+      auto E = getCommonTypeArray(Ctx, EPIX.ExceptionSpec.Exceptions,
+                                  EPIY.ExceptionSpec.Exceptions);
+      EPIX.ExceptionSpec.Exceptions = E;
+      return Ctx.getFunctionType(R, P, EPIX);
+    } else {
----------------
rsmith wrote:
> This seems a bit suspicious: `getCommonTypeArray` assumes the two arrays contain the same types in the same order, but if the arrays can be different sizes, is it really correct to assume that they contain the same types if they're the same size?
> 
> Can we make this work the same way that the conditional expression handling deals with differing lists of exception types?
Handled this with a new `ASTContext::mergeTypeLists` that does what the conditional expression handling did, while getting the common sugar of types when they occur in both lists.


================
Comment at: clang/lib/AST/ASTContext.cpp:11861
+               *IY = cast<InjectedClassNameType>(Y);
+    assert(IX->getDecl() == IY->getDecl());
+    return Ctx.getInjectedClassNameType(
----------------
rsmith wrote:
> I think this should probably be a `declaresSameEntity` check: we might be comparing injected class names from two different merged modules with two different declarations for the same class definition. The right declaration to use is the one that is chosen to be "the" definition.
Implemented this with a new getCommonDecl function, which for now will just fall back to the canonical decl if they differ.

Wonder if there is a cheap way to determine which of the two decls was declared earlier. Have to look this up, but would a simple pointer comparison be guaranteed to work? Eg would the earliest declaration, since being created earlier and allocated earlier, be guaranteed by design to have a lower (or greater) pointer value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283