-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[Clang] Add __builtin_common_type #99473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
d6903da
6b8d230
a912a76
f418c26
eb8afc0
271232b
4e3cd04
3b570f8
656d12c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3058,6 +3058,141 @@ void Sema::NoteAllFoundTemplates(TemplateName Name) { | |
} | ||
} | ||
|
||
static std::optional<QualType> commonTypeImpl(Sema &S, | ||
TemplateName BaseTemplate, | ||
SourceLocation TemplateLoc, | ||
ArrayRef<TemplateArgument> Ts) { | ||
auto lookUpCommonType = [&](TemplateArgument T1, | ||
TemplateArgument T2) -> std::optional<QualType> { | ||
// Don't bother looking for other specializations if both types are | ||
// builtins - users aren't allowed to specialize for them | ||
Comment on lines
+3084
to
+3085
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we diagnose the case where users do this, please? Similarly for specialising There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do that, but I don't think that's part of this project. AFAICT this requires tinkering with the template specialization code and check whether a specialization is allowed to exist. To me that seems like an entire project on its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to handle the first case in this patch because it is currently silently changing the outcome of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this any different from all the other cases where we switched to builtins, which results in potential breaks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. There are only a handful of cases that this applies to, but they should also break loudly. In that case, I'd prefer it if getting that diagnostic logic into Clang preceded submitting this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the precedent I don't think it's reasonable to block this patch on diagnosing some edge cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is only precedent due to it not having been previously recognised as an issue. It has now been identified and should be addressed, instead of increasing the problem's surface area. We should do things right, not wrong because "they were done that way previously". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it would make sense for such a diagnostic to work today, even without the builtin. For example, it would be nice for the following code to be diagnosed with a warning (or an error, whatever): #include <type_traits>
template <>
struct std::common_type<int, int> { using type = long; }; // should warn
struct Foo { };
template <>
struct std::common_type<Foo> { using type = int; }; // should warn
// etc... I don't think this should be tied to the implementation of the builtin, i.e. it should be diagnosed where the user writes the invalid specialization, not where we use |
||
if (T1.getAsType()->isBuiltinType() && T2.getAsType()->isBuiltinType()) | ||
return commonTypeImpl(S, BaseTemplate, TemplateLoc, {T1, T2}); | ||
Sirraide marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
TemplateArgumentListInfo Args; | ||
Args.addArgument(TemplateArgumentLoc( | ||
T1, S.Context.getTrivialTypeSourceInfo(T1.getAsType()))); | ||
Args.addArgument(TemplateArgumentLoc( | ||
T2, S.Context.getTrivialTypeSourceInfo(T2.getAsType()))); | ||
QualType BaseTemplateInst = | ||
S.CheckTemplateIdType(BaseTemplate, TemplateLoc, Args); | ||
if (S.RequireCompleteType(TemplateLoc, BaseTemplateInst, | ||
diag::err_incomplete_type)) | ||
return std::nullopt; | ||
if (QualType Type = S.getTypeMember("type", BaseTemplateInst); | ||
!Type.isNull()) { | ||
return Type; | ||
} | ||
return std::nullopt; | ||
}; | ||
|
||
// Note A: For the common_type trait applied to a template parameter pack T of | ||
// types, the member type shall be either defined or not present as follows: | ||
switch (Ts.size()) { | ||
|
||
// If sizeof...(T) is zero, there shall be no member type. | ||
case 0: | ||
return std::nullopt; | ||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// If sizeof...(T) is one, let T0 denote the sole type constituting the | ||
// pack T. The member typedef-name type shall denote the same type, if any, as | ||
// common_type_t<T0, T0>; otherwise there shall be no member type. | ||
case 1: | ||
return lookUpCommonType(Ts[0], Ts[0]); | ||
|
||
// If sizeof...(T) is two, let the first and second types constituting T be | ||
// denoted by T1 and T2, respectively, and let D1 and D2 denote the same types | ||
// as decay_t<T1> and decay_t<T2>, respectively. | ||
case 2: { | ||
QualType T1 = Ts[0].getAsType(); | ||
QualType T2 = Ts[1].getAsType(); | ||
QualType D1 = S.BuiltinDecay(T1, {}); | ||
QualType D2 = S.BuiltinDecay(T2, {}); | ||
|
||
// If is_same_v<T1, D1> is false or is_same_v<T2, D2> is false, let C denote | ||
// the same type, if any, as common_type_t<D1, D2>. | ||
if (!S.Context.hasSameType(T1, D1) || !S.Context.hasSameType(T2, D2)) { | ||
return lookUpCommonType(D1, D2); | ||
} | ||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Otherwise, if decay_t<decltype(false ? declval<D1>() : declval<D2>())> | ||
// denotes a valid type, let C denote that type. | ||
{ | ||
auto CheckConditionalOperands = | ||
[&](bool ConstRefQual) -> std::optional<QualType> { | ||
EnterExpressionEvaluationContext UnevaluatedContext( | ||
S, Sema::ExpressionEvaluationContext::Unevaluated); | ||
Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true); | ||
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl()); | ||
|
||
// false | ||
OpaqueValueExpr CondExpr({}, S.Context.BoolTy, | ||
ExprValueKind::VK_PRValue); | ||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ExprResult Cond = &CondExpr; | ||
|
||
auto EVK = | ||
ConstRefQual ? ExprValueKind::VK_LValue : ExprValueKind::VK_PRValue; | ||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (ConstRefQual) { | ||
D1.addConst(); | ||
D2.addConst(); | ||
} | ||
|
||
// declval<D1>() | ||
OpaqueValueExpr LHSExpr(TemplateLoc, D1, EVK); | ||
ExprResult LHS = &LHSExpr; | ||
|
||
// declval<D2>() | ||
OpaqueValueExpr RHSExpr(TemplateLoc, D2, EVK); | ||
ExprResult RHS = &RHSExpr; | ||
|
||
ExprValueKind VK = VK_PRValue; | ||
ExprObjectKind OK = OK_Ordinary; | ||
|
||
// decltype(false ? declval<D1>() : declval<D2>()) | ||
QualType Result = | ||
S.CheckConditionalOperands(Cond, LHS, RHS, VK, OK, TemplateLoc); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I think we can call But the question I have is, should we factor out the part of One one hand, doing surgery to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT we'd have to do that for all the extensions as well, which seems like it'd be a lot of work. We'd also only save the SFINAE context and the opaque values, which seems to me like not that much code. It'd probably be a bit faster, but I'm not sure it's that significant. |
||
|
||
if (Result.isNull() || SFINAE.hasErrorOccurred()) | ||
return std::nullopt; | ||
|
||
// decay_t<decltype(false ? declval<D1>() : declval<D2>())> | ||
return S.BuiltinDecay(Result, TemplateLoc); | ||
}; | ||
|
||
if (auto Res = CheckConditionalOperands(false)) | ||
return Res; | ||
|
||
// Let: | ||
// CREF(A) be add_lvalue_reference_t<const remove_reference_t<A>>, | ||
// COND-RES(X, Y) be | ||
// decltype(false ? declval<X(&)()>()() : declval<Y(&)()>()()). | ||
Sirraide marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// C++20 only | ||
// Otherwise, if COND-RES(CREF(D1), CREF(D2)) denotes a type, let C denote | ||
// the type decay_t<COND-RES(CREF(D1), CREF(D2))>. | ||
if (!S.Context.getLangOpts().CPlusPlus20) | ||
return std::nullopt; | ||
return CheckConditionalOperands(true); | ||
} | ||
} | ||
|
||
// If sizeof...(T) is greater than two, let T1, T2, and R, respectively, | ||
// denote the first, second, and (pack of) remaining types constituting T. Let | ||
// C denote the same type, if any, as common_type_t<T1, T2>. If there is such | ||
// a type C, the member typedef-name type shall denote the same type, if any, | ||
// as common_type_t<C, R...>. Otherwise, there shall be no member type. | ||
default: { | ||
std::optional<QualType> Result = Ts[Ts.size() - 1].getAsType(); | ||
for (size_t i = Ts.size() - 1; i != 0; --i) { | ||
Result = lookUpCommonType(Ts[i - 1].getAsType(), *Result); | ||
Sirraide marked this conversation as resolved.
Show resolved
Hide resolved
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!Result) | ||
return std::nullopt; | ||
} | ||
return Result; | ||
} | ||
} | ||
} | ||
|
||
static QualType | ||
checkBuiltinTemplateIdType(Sema &SemaRef, BuiltinTemplateDecl *BTD, | ||
ArrayRef<TemplateArgument> Converted, | ||
|
@@ -3114,7 +3249,7 @@ checkBuiltinTemplateIdType(Sema &SemaRef, BuiltinTemplateDecl *BTD, | |
TemplateLoc, SyntheticTemplateArgs); | ||
} | ||
|
||
case BTK__type_pack_element: | ||
case BTK__type_pack_element: { | ||
// Specializations of | ||
// __type_pack_element<Index, T_1, ..., T_N> | ||
// are treated like T_Index. | ||
|
@@ -3140,6 +3275,29 @@ checkBuiltinTemplateIdType(Sema &SemaRef, BuiltinTemplateDecl *BTD, | |
int64_t N = Index.getExtValue(); | ||
return Ts.getPackAsArray()[N].getAsType(); | ||
} | ||
|
||
case BTK__common_type: { | ||
assert(Converted.size() == 4); | ||
if (Converted[0].isDependent() || Converted[1].isDependent() || | ||
Converted[2].isDependent() || Converted[3].isDependent()) | ||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return Context.getCanonicalTemplateSpecializationType(TemplateName(BTD), | ||
Converted); | ||
|
||
TemplateName BaseTemplate = Converted[0].getAsTemplate(); | ||
TemplateName HasTypeMember = Converted[1].getAsTemplate(); | ||
QualType HasNoTypeMember = Converted[2].getAsType(); | ||
ArrayRef<TemplateArgument> Ts = Converted[3].getPackAsArray(); | ||
if (auto CT = commonTypeImpl(SemaRef, BaseTemplate, TemplateLoc, Ts)) { | ||
TemplateArgumentListInfo TAs; | ||
TAs.addArgument(TemplateArgumentLoc( | ||
TemplateArgument(*CT), SemaRef.Context.getTrivialTypeSourceInfo( | ||
*CT, TemplateArgs[1].getLocation()))); | ||
|
||
return SemaRef.CheckTemplateIdType(HasTypeMember, TemplateLoc, TAs); | ||
} | ||
return HasNoTypeMember; | ||
Comment on lines
+3313
to
+3315
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing wrapping the result in a TemplateSpecializationType, just like the other builtin templates do, because users of CheckTemplateId assume that it returns a TST of whatever TemplateName it is checking. Worse, in the negative case, HasNoTypeMember can be something which is not a TST at all, and this will lead to crashes when we try to build a TypeLoc with the wrong kind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand. |
||
} | ||
} | ||
llvm_unreachable("unexpected BuiltinTemplateDecl!"); | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.