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

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

rsmith added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12177
+    return X;
+  assert(X->getCanonicalDecl() == Y->getCanonicalDecl());
+  // FIXME: Could return the earliest declaration between those two.
----------------



================
Comment at: clang/lib/AST/ASTContext.cpp:12205
+  llvm::DenseMap<QualType, unsigned> Found;
+  for (auto Ts : {X, Y})
+    for (QualType T : Ts) {
----------------
Please add braces to this outer  `for`.


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


================
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());
----------------
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?


================
Comment at: clang/lib/AST/ASTContext.cpp:12327-12328
+    return Ctx.getLValueReferenceType(getCommonPointeeType(Ctx, PX, PY),
+                                      PX->isSpelledAsLValue() &&
+                                          PY->isSpelledAsLValue());
+  }
----------------
If either of them was spelled as an lvalue then the pointee type was spelled as either an lvalue reference or a non-reference for that input type, and we'll strip off the spelling-as-rvalue-reference part of the other type when finding the common type. The only case in which I think it makes sense to retain the "spelled as rvalue" flag would be if *both* sides were lvalue references spelled as rvalue references.


================
Comment at: clang/lib/AST/ASTContext.cpp:12378-12379
+
+    if (EPIX.EllipsisLoc != EPIY.EllipsisLoc)
+      EPIX.EllipsisLoc = SourceLocation();
+    EPIX.HasTrailingReturn = EPIX.HasTrailingReturn && EPIY.HasTrailingReturn;
----------------
Can we cope with a variadic function with no ellipsis location? I'd expect that to confuse some parts of Clang. Picking one of the two arbitrarily isn't great but may be the best we can do.


================
Comment at: clang/lib/AST/ASTContext.cpp:12464
+                                TY->getTemplateName()),
+        As, QualType(TX, 0));
+  }
----------------
No-op change, but this would make it a bit more obvious to a reader why it's OK to pass in `TX` here.


================
Comment at: clang/lib/AST/ASTContext.cpp:11578
+  default:
+    return X;
+  }
----------------
mizvekov wrote:
> 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.
This will need to deal with all the cases that can arrive here. If it sees resolved template arguments, then this includes `Type`, `Declaration`, `NullPtr`, `Integral`, `Template`, `TemplateExpansion`, and `Pack`. If it sees unresolved template arguments, then this includes at least `Type`, `Expression`, `Template`, and `TemplateExpansion`. This function currently doesn't cover either set, so I strongly suspect that this `llvm_unreachable` is actually reachable.

Can we just `return X;` on all the cases we don't currently handle?


================
Comment at: clang/lib/AST/ASTContext.cpp:11861
+               *IY = cast<InjectedClassNameType>(Y);
+    assert(IX->getDecl() == IY->getDecl());
+    return Ctx.getInjectedClassNameType(
----------------
mizvekov wrote:
> 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?
No, there's no guarantee on pointer ordering. While we do use slab allocation, we allocate multiple slabs and there's no ordering between slabs. (Also declarations can be read from an AST file in any order.) There's no good way to find which of two declarations was declared first without a linear scan; see `NamedDecl::declarationReplaces` for existing code doing that scan. It might be that we need the scan rarely enough that we can afford to do it.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:1517
     if (DeducedType.isNull())
       return ExprError();
 
----------------
It'd be nice to add an `assert(Result == TDK_AlreadyDiagnosed);` here now that we can do so.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:2059
     if (DeducedType.isNull())
       return ExprError();
     AllocType = DeducedType;
----------------
Likewise it'd be nice to add an `assert(Result == TDK_AlreadyDiagnosed);` here.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4583-4586
 /// \param DependentDeductionDepth Set if we should permit deduction in
 ///        dependent cases. This is necessary for template partial ordering with
 ///        'auto' template parameters. The value specified is the template
 ///        parameter depth at which we should perform 'auto' deduction.
----------------
This comment is out of date.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4715-4716
+             "substituting template parameter for 'auto' failed");
+      if (!Result.isNull())
+        Deduced[0] = DeducedTemplateArgument(Result);
+      if (auto TDK = DeduceTemplateArgumentsFromCallArgument(
----------------
I do like this approach to repeated deduction, but I'm not sure that it actually works. For example:

```
void f();
void g();
void g(int);
auto h(bool b) {
  if (b) return f;
  return g;
}
```
I think this is required to fail to compile, because we can't deduce the return type from the returned expression `g`. But I think what will happen here is that we'll say that `g` is a non-deduced context and then succeed because we pre-populated the deduced type from the `void(*)()` from `f`.

Instead, how about not pre-populating `Deduced[0]`, and instead "deducing" the template parameter from `Result` after we check for the `TDK_Incomplete` case below? That should still let us reuse the logic for merging deductions.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4732
+        return TDK_AlreadyDiagnosed;
+      if (!Result.isNull() && !Context.hasSameType(Result, DeducedType)) {
+        Info.FirstArg = Result;
----------------
Can we use `checkDeducedTemplateArguments` here to unify the type sugar?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283