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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
