rsmith added inline comments.
Herald added a subscriber: carlosgalvezp.
================
Comment at: clang/include/clang/AST/Type.h:4968
QualType getDeducedType() const {
- return !isCanonicalUnqualified() ? getCanonicalTypeInternal() : QualType();
+ return DeducedAsType.isNull() ? QualType() : DeducedAsType;
}
----------------
================
Comment at: clang/lib/AST/ASTImporter.cpp:3275
+ !DeducedT.isNull()
+ ? dyn_cast<RecordType>(DeducedT->getCanonicalTypeInternal())
+ : nullptr) {
----------------
================
Comment at: clang/lib/AST/ASTImporter.cpp:3285
}
- if (const TypedefType *TypedefT =
- dyn_cast<TypedefType>(FromFPT->getReturnType())) {
- TypedefNameDecl *TD = TypedefT->getDecl();
+ if (const auto *TypedefT = dyn_cast<TypedefType>(FromFPT->getReturnType())) {
+ const TypedefNameDecl *TD = TypedefT->getDecl();
----------------
(Preparing for D112374)
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1154
if (!ParamFunction || !ArgFunction)
return Param == Arg;
----------------
The `==` comparisons here and below should presumably be `hasSameType`
comparisons now that we're preserving sugar.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1234-1250
llvm::SmallPtrSet<const RecordType *, 8> Visited;
SmallVector<const RecordType *, 8> ToVisit;
// We iterate over this later, so we have to use MapVector to ensure
// determinism.
llvm::MapVector<const RecordType *, SmallVector<DeducedTemplateArgument, 8>>
Matches;
----------------
I think we should:
* track `ToVisit` as a vector of `QualType`, so we preserve the type sugar as
written in base specifier lists
* track `Visited` as a set of (canonical) `const CXXRecordDecl*`s rather than
relying on `RecordType`s always being canonical (they currently are, but I
don't think that's intended to be guaranteed)
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1385-1388
+ P = P.getUnqualifiedType();
// - If A is a cv-qualified type, A is replaced by the cv-unqualified
// version of A.
+ A = A.getUnqualifiedType();
----------------
Do we need to use `Context.getUnqualifiedArrayType` here, for cases like:
```
typedef const int CInt;
// Partial ordering should deduce `T = int` from both parameters here,
// even though `T = const int` might make more sense for the first one.
template<int N> void f(CInt (&)[N], int*);
template<int N, typename T> void f(T (&)[N], T*);
CInt arr[5];
void g() { f(arr, arr); }
```
? I think `getUnqualifiedType()` on `CInt[N]` will not remove the indirect
`const` qualifier.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1437
// top level, so they can be matched with the qualifiers on the parameter.
- if (isa<ArrayType>(Arg)) {
+ if (isa<ArrayType>(A)) {
Qualifiers Quals;
----------------
`A` could be sugar for an array type here.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1546-1568
+ QualType CanP = P.getCanonicalType();
+ // If the parameter type is not dependent, there is nothing to deduce.
+ if (!P->isDependentType()) {
+ if (TDF & TDF_SkipNonDependent)
return Sema::TDK_Success;
+
+ QualType CanA = A.getCanonicalType();
----------------
I think we can excise `CanA` and `CanP` entirely here so people aren't tempted
to use them and lose sugar.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
- return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+ return ParDesug == ArgDesug ? Sema::TDK_Success
+ : Sema::TDK_NonDeducedMismatch;
----------------
mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > This looks wrong to me: we should be comparing the types, not how they're
> > > written. `Context.hasSameType(Param, Arg)` (or
> > > `Context.hasSameUnqualifiedType(Param, Arg)` in the
> > > `TDF_IgnoreQualifiers` case) would be appropriate here.
> > You are right, but the reason we don't get into any troubles here is
> > because this is dead code anyway, the non-dependent case will always be
> > handled above :)
> >
> > Although perhaps, I wonder if we should dig down into non-dependent types
> > anyway, in case the types are too complex and it's not immediately obvious
> > what does not match, we could perhaps improve the diagnostic?
> >
> > I will experiment a little bit with this idea.
> Here are the results of this experiment:
> ```
> error: 'note' diagnostics expected but not seen:
> File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: could not match
> 'void (I) noexcept(false)' (aka 'void (int) noexcept(false)') against 'void
> (int) noexcept'
> error: 'note' diagnostics seen but not expected:
> File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: candidate template
> ignored: failed template argument deduction
>
> error: 'note' diagnostics expected but not seen:
> File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template
> ignored: could not match 'auto ()' against 'int ()'
> File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template
> ignored: could not match 'auto ()' against 'void ()'
> error: 'note' diagnostics seen but not expected:
> File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template
> ignored: could not match 'auto' against 'int'
> File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template
> ignored: could not match 'auto' against 'void'
>
> error: 'note' diagnostics expected but not seen:
> File SemaCXX\pass-object-size.cpp Line 62 (directive at
> SemaCXX\pass-object-size.cpp:69): candidate address cannot be taken because
> parameter 1 has pass_object_size attribute
> File SemaCXX\pass-object-size.cpp Line 56 (directive at
> SemaCXX\pass-object-size.cpp:74): candidate address cannot be taken because
> parameter 1 has pass_object_size attribute
> File SemaCXX\pass-object-size.cpp Line 62 (directive at
> SemaCXX\pass-object-size.cpp:75): candidate address cannot be taken because
> parameter 1 has pass_object_size attribute
> error: 'note' diagnostics seen but not expected:
> File SemaCXX\pass-object-size.cpp Line 56: candidate template ignored:
> failed template argument deduction
> File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored:
> failed template argument deduction
> File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored:
> failed template argument deduction
>
> error: 'note' diagnostics expected but not seen:
> File SemaTemplate\deduction.cpp Line 316: deduced non-type template
> argument does not have the same type as the corresponding template parameter
> ('std::nullptr_t' vs 'int *')
> File SemaTemplate\deduction.cpp Line 323: values of conflicting types
> error: 'note' diagnostics seen but not expected:
> File SemaTemplate\deduction.cpp Line 275: candidate template ignored: could
> not match 'const int' against 'int'
> File SemaTemplate\deduction.cpp Line 316: candidate template ignored: could
> not match 'int *' against 'std::nullptr_t'
> File SemaTemplate\deduction.cpp Line 323: candidate template ignored: could
> not match 'int *' against 'std::nullptr_t'
>
> error: 'note' diagnostics expected but not seen:
> File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template
> ignored: could not match 'void ()' against 'void (float *)'
> File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template
> ignored: could not match 'void (int *)' against 'void (float *)'
> error: 'note' diagnostics seen but not expected:
> File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template
> ignored: could not match 'int' against 'float'
> File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template
> ignored: failed template argument deduction
> ```
>
> It's interesting to note that it reveals several cases we give too generic
> 'failed template argument deduction' errors, like the different noexcept
> specifications, function prototypes with different amount of parameters, and
> the 'pass_object_size attribute' case.
Hm, this doesn't seem to be an improvement; I think it's better to present the
larger non-matching types and use `%diff` in the diagnostic to highlight the
part that differs. Especially because we don't provide a source location for
(eg) where in the type we found a `float`, providing the more complete context
of (eg) `void (float*)` seems more useful.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110216/new/
https://reviews.llvm.org/D110216
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
