[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
jtbandes added a comment. Please take another look when you get a chance :) Repository: rC Clang https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
jtbandes updated this revision to Diff 133440.
jtbandes added a comment.
Using a slightly more invasive fix. I haven't come up with any other test cases
that exhibit the problem, which makes me unsure this fix is needed in all these
locations. Maybe someone with more knowledge of this function can advise.
Repository:
rC Clang
https://reviews.llvm.org/D40284
Files:
lib/Sema/SemaTemplateDeduction.cpp
test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
func(foo()); // expected-error {{no matching function}}
}
}
+
+namespace test4 {
+ // expected-note@+1 {{candidate template ignored: could not match 'int [N]' against 'int []'}}
+ template void f1(int ()[N]);
+ template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+ }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1289,27 +1289,34 @@
return Sema::TDK_Success;
}
- // Set up the template argument deduction information for a failure.
- Info.FirstArg = TemplateArgument(ParamIn);
- Info.SecondArg = TemplateArgument(ArgIn);
-
// If the parameter is an already-substituted template parameter
// pack, do nothing: we don't know which of its arguments to look
// at, so we have to wait until all of the parameter packs in this
// expansion have arguments.
if (isa(Param))
return Sema::TDK_Success;
+ // Use this when returning a failure result in order to fill in the
+ // corresponding failure info. Don't use this when returning the
+ // result of a recursive call, as the callee will have already set
+ // their own failure info.
+ const auto WithFailureInfo =
+[, , ](Sema::TemplateDeductionResult Result) {
+ Info.FirstArg = TemplateArgument(ParamIn);
+ Info.SecondArg = TemplateArgument(ArgIn);
+ return Result;
+};
+
// Check the cv-qualifiers on the parameter and argument types.
CanQualType CanParam = S.Context.getCanonicalType(Param);
CanQualType CanArg = S.Context.getCanonicalType(Arg);
if (!(TDF & TDF_IgnoreQualifiers)) {
if (TDF & TDF_ParamWithReferenceType) {
if (hasInconsistentOrSupersetQualifiersOf(Param, Arg))
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
} else if (!IsPossiblyOpaquelyQualifiedType(Param)) {
if (Param.getCVRQualifiers() != Arg.getCVRQualifiers())
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
}
// If the parameter type is not dependent, there is nothing to deduce.
@@ -1319,9 +1326,8 @@
(TDF & TDF_AllowCompatibleFunctionType)
? !S.isSameOrCompatibleFunctionType(CanParam, CanArg)
: Param != Arg;
-if (NonDeduced) {
- return Sema::TDK_NonDeducedMismatch;
-}
+if (NonDeduced)
+ return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
}
return Sema::TDK_Success;
}
@@ -1366,7 +1372,10 @@
Arg = Arg.getUnqualifiedType();
}
- return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+ if (Param == Arg)
+return Sema::TDK_Success;
+
+ return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
}
// _Complex T [placeholder extension]
@@ -1377,7 +1386,7 @@
ComplexArg->getElementType(),
Info, Deduced, TDF);
- return Sema::TDK_NonDeducedMismatch;
+ return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
// _Atomic T [extension]
case Type::Atomic:
@@ -1387,7 +1396,7 @@
AtomicArg->getValueType(),
Info, Deduced, TDF);
- return Sema::TDK_NonDeducedMismatch;
+ return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
// T *
case Type::Pointer: {
@@ -1398,7 +1407,7 @@
= Arg->getAs()) {
PointeeType = PointerArg->getPointeeType();
} else {
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
}
unsigned SubTDF = TDF & (TDF_IgnoreQualifiers | TDF_DerivedClass);
@@ -1413,7 +1422,7 @@
const LValueReferenceType *ReferenceArg =
Arg->getAs();
if (!ReferenceArg)
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
return DeduceTemplateArgumentsByTypeMatch(S,
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
rsmith added a comment. I would prefer that we make the more-invasive fix, and make each error case within template argument deduction set all the deduction failure information. (Consider factoring out the error setup into helper functions too.) The current approach is a maintenance problem in addition to creating incorrect diagnostics, as it makes it very hard to see what parameters are actually intended to be provided with each of the different forms of deduction failure. https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
lichray added a comment. I'm not confident enough about the effects of this patch with only one test; the last patch also passes the test. I hope I can understand the effects better. https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
jtbandes added a comment. Bump :) https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
jtbandes updated this revision to Diff 124033.
jtbandes added a comment.
@erik.pilkington Updated to use a wrapper function. This is definitely less
invasive, but it could defeat some optimizations (any approach that checks the
return value will defeat tail recursion, I suppose...)
https://reviews.llvm.org/D40284
Files:
lib/Sema/SemaTemplateDeduction.cpp
test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
func(foo()); // expected-error {{no matching function}}
}
}
+
+namespace test4 {
+ // expected-note@+1 {{candidate template ignored: could not match 'int [N]'
against 'int []'}}
+ template void f1(int ()[N]);
+ template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+ }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1056,6 +1056,12 @@
return false;
}
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound);
+
/// \brief Deduce the template arguments by comparing the parameter type and
/// the argument type (C++ [temp.deduct.type]).
///
@@ -1080,15 +1086,34 @@
/// \returns the result of template argument deduction so far. Note that a
/// "success" result means that template argument deduction has not yet failed,
/// but it may still fail, later, for other reasons.
-static Sema::TemplateDeductionResult
-DeduceTemplateArgumentsByTypeMatch(Sema ,
- TemplateParameterList *TemplateParams,
- QualType ParamIn, QualType ArgIn,
- TemplateDeductionInfo ,
-SmallVectorImpl ,
- unsigned TDF,
- bool PartialOrdering,
- bool DeducedFromArrayBound) {
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound) {
+ // Because DeduceTemplateArgumentsByTypeMatchInner is recursive, and tends to
+ // modify Info.FirstArg and SecondArg even when deduction succeeds, save the
+ // original values and restore them if no error occurred.
+ const auto OriginalFirstArg = Info.FirstArg;
+ const auto OriginalSecondArg = Info.SecondArg;
+
+ const auto Result = DeduceTemplateArgumentsByTypeMatchInner(
+ S, TemplateParams, ParamIn, ArgIn, Info, Deduced, TDF, PartialOrdering,
+ DeducedFromArrayBound);
+
+ if (Result == Sema::TDK_Success) {
+Info.FirstArg = OriginalFirstArg;
+Info.SecondArg = OriginalSecondArg;
+ }
+ return Result;
+}
+
+/// \see DeduceTemplateArgumentsByTypeMatch()
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound) {
// We only want to look at the canonical types, since typedefs and
// sugar are not part of template argument deduction.
QualType Param = S.Context.getCanonicalType(ParamIn);
Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
func(foo()); // expected-error {{no matching function}}
}
}
+
+namespace test4 {
+ // expected-note@+1 {{candidate template ignored: could not match 'int [N]' against 'int []'}}
+ template void f1(int ()[N]);
+ template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+ }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1056,6 +1056,12 @@
return false;
}
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
erik.pilkington added a comment. This looks correct, but I definitely agree that RAII would make this a lot nicer. Have you considered adding a `CancelableSaveAndRestore` or something to SaveAndRestore.h? It seems useful and generic enough to make it worthwhile. Otherwise, you could just write your own RAII object special-cased to handle this. A less intrusive way of doing this might be to wrap calls to this function with another that checks if the return value is TDK_Success, and if so restores these fields. Comment at: lib/Sema/SemaTemplateDeduction.cpp:1376 if (const ComplexType *ComplexArg = Arg->getAs()) return DeduceTemplateArgumentsByTypeMatch(S, TemplateParams, cast(Param)->getElementType(), What if this return TDK_Success? https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction
jtbandes added a reviewer: rsmith. jtbandes added a subscriber: rsmith. jtbandes added a comment. Adding @rsmith for review based on the commit history of SemaTemplateDeduction.cpp :) https://reviews.llvm.org/D40284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
