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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.
Herald added a project: All.


================
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);
----------------
What should happen if there's a qualifier difference other than a CVR qualifier? Eg, address space or ObjC lifetime.


================
Comment at: clang/lib/AST/ASTContext.cpp:11578
+  default:
+    return X;
+  }
----------------
For `TemplateArgument::ArgKind::Declaration`, I think it'd make sense to take the common type of the `getParamTypeForDecl`s.


================
Comment at: clang/lib/AST/ASTContext.cpp:11603
+             ? X->getQualifier()
+             : Ctx.getCanonicalNestedNameSpecifier(X->getQualifier());
+}
----------------
Might be worth adding a `FIXME` to keep as much common NNS sugar as we can?


================
Comment at: clang/lib/AST/ASTContext.cpp:11632-11633
+
+static QualType getCommonCanonicalType(ASTContext &Ctx, const Type *X,
+                                       const Type *Y) {
+  Type::TypeClass TC = X->getTypeClass();
----------------
I don't understand this function name: there's only one canonical representation of a type, so this seems paradoxical. I think `getCommonDesugaredType` is probably a clearer name for what this is doing.


================
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 {
----------------
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?


================
Comment at: clang/lib/AST/ASTContext.cpp:11861
+               *IY = cast<InjectedClassNameType>(Y);
+    assert(IX->getDecl() == IY->getDecl());
+    return Ctx.getInjectedClassNameType(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283