[PATCH] C++1y: Implement n3599
Richard Smith
richard at metafoo.co.uk
Fri Oct 4 17:12:12 PDT 2013
This looks really good. Just a few more minor things and this will be ready
to commit.
+ // The second template parameter must be a parameter pack with the
+ // first template parameter as its type.
+ if (PmType && PmArgs &&
+ !PmType->isTemplateParameterPack() &&
+ PmArgs->isTemplateParameterPack()) {
+ const TemplateTypeParmType *TArgs =
+ dyn_cast<const
TemplateTypeParmType>(PmArgs->getType().getTypePtr());
Use PmArgs->getType()->getAs<TemplateTypeParmType>() here, to cope with
type sugar properly. Testcase (which should be accepted, and which I
believe you'll currently reject):
template<typename T> using X = T;
template<typename T, X<T>... Chars> void operator""_x();
+ if (TArgs && TArgs->getDecl() == PmType){
Space before {, please.
+ if (ActiveTemplateInstantiations.empty() &&
!getLangOpts().GNUMode)
+ Diag(FnDecl->getLocation(),
diag::ext_string_literal_operator_template);
Please remove the GNUMode check here (we want to be able to warn on this
extension even in -std=gnu++1y mode).
+ if (FunctionTemplateDecl *FD = dyn_cast<FunctionTemplateDecl>(D)) {
+ TemplateParameterList *Params=FD->getTemplateParameters();
+ if (Params->size() == 1)
+ IsTemplate=true;
Please add spaces around the '=' here (2x).
On Fri, Oct 4, 2013 at 11:39 AM, Hristo Venev <mustrumr97 at gmail.com> wrote:
> I think I fixed most bugs from the first version of the patch.
> I tried checking if the parameters' type hasQualifiers() on definition and
> it's always false. Why?
>
Oh, sorry. The qualifiers are dropped when forming the parameter's type; we
don't need to check for them.
> I don't think the type argument should be const-qualified so I'll ask the
> gcc folks what they think.
> BuildLiteralOperatorCall is given LitEndTok so StringTokLocs.back() should
> be right.
>
OK, makes sense. UserDefinedLiteral::getLocStart() seems to do the wrong
thing here, though -- it assumes that for LOK_Template the start and end
locations are the same, which isn't true for a string UDL (eg "foo"
"bar"_x). Ideally, we should store the source locations for all of the
string literal tokens involved in the UserDefinedLiteral, but I'm happy to
defer that work for now, to keep this patch simpler.
The TokLoc change is because Tok.getLocation() is always called. Now it's
> only called at one place.
> I also added some raw string/unicode tests and they seem to work as
> expected.
>
Thanks!
On Fri, Oct 4, 2013 at 12:06 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> I believe you'll need to update LookupLiteralOperator to handle this new
>> form appropriately. In particular, it looks like you'll currently reject
>> cases like:
>>
>> template<typename T, T...> int &operator""_x(); // #1
>> float &operator""_x(const char*); // #2
>> int &a = "foo"_x; // ok, calls #1
>> float &b = 123_x; // ok, calls #2
>>
>> It's probably best to repurpose the AllowRawAndTemplate flag into an enum
>> which specifies what form of literal operator you look for.
>>
>>
>> + }else if (Params->size() == 2) {
>>
>> Space before 'else'
>>
>> + // The second template parameter must be a parameter pack with
>> the
>> + // first template parameter as its type.
>> + if (PmType && PmArgs &&
>> + !PmType->isTemplateParameterPack() &&
>> + PmArgs->isTemplateParameterPack()) {
>> + const TemplateTypeParmType *TArgs =
>> + dyn_cast<const
>> TemplateTypeParmType>(PmArgs->getType().getTypePtr());
>>
>> You're dropping type qualifiers here. <typename T, const T...> is not an
>> acceptabe template-parameter-list.
>>
>> + if(TArgs && TArgs->getDecl() == PmType){
>>
>> Space after 'if'.
>>
>> + Valid = true;
>> + if (!getLangOpts().CPlusPlus1y)
>> + Diag(FnDecl->getLocation(),
>> diag::warn_string_literal_operator_template);
>>
>> This should be an ext_... not a warn_... (and should be a GNU warning not
>> a C++1y warning).
>>
>> + }
>> + }
>>
>>
>> @@ -1538,12 +1536,53 @@ Sema::ActOnStringLiteral(const Token *StringToks,
>> unsigned NumStringToks,
>> + DeclarationName OpName =
>> + Context.DeclarationNames.getCXXLiteralOperatorName(UDSuffix);
>> + DeclarationNameInfo OpNameInfo(OpName, UDSuffixLoc);
>> + OpNameInfo.setCXXLiteralOperatorNameLoc(UDSuffixLoc);
>> +
>> + QualType ArgTy[2];
>> + ArgTy[0]=Context.getArrayDecayedType(StrTy);
>> + ArgTy[1]=SizeType;
>>
>> Spaces around '='. But please write this using braced initialization.
>>
>> +
>> + LookupResult R(*this, OpName, UDSuffixLoc, LookupOrdinaryName);
>> + switch (LookupLiteralOperator(UDLScope, R, llvm::makeArrayRef(ArgTy,
>> 2),
>> + /*AllowRawAndTemplate*/true))
>> {
>> +
>> + case LOLR_Cooked:
>> + case LOLR_Raw: {
>>
>> llvm_unreachable on the Raw case; that can't happen for a string.
>>
>> + llvm::APInt Len(Context.getIntWidth(SizeType),
>> Literal.GetNumStringChars());
>> + IntegerLiteral *LenArg = IntegerLiteral::Create(Context, Len,
>> SizeType,
>> + StringTokLocs[0]);
>> + Expr *Args[] = { Lit, LenArg };
>> +
>> + return BuildLiteralOperatorCall(R, OpNameInfo, Args,
>> StringTokLocs.back());
>> + }
>> +
>> + case LOLR_Template: {
>> + TemplateArgumentListInfo ExplicitArgs;
>> +
>> + unsigned CharBits = Context.getIntWidth(CharTy);
>> + bool CharIsUnsigned = CharTy->isUnsignedIntegerType();
>> + llvm::APSInt Value(CharBits, CharIsUnsigned);
>> +
>> + TemplateArgument TypeArg(CharTy);
>>
>> GCC uses the const-qualified element type here (it's not what I intended
>> when I wrote the paper, but it matches the wording, and it's what we need
>> to do for GCC compatibility). Either use CharTyConst here or persuade the
>> GCC folks to change their implementation =)
>>
>> + TemplateArgumentLocInfo
>> TypeArgInfo(Context.getTrivialTypeSourceInfo(CharTy));
>> + ExplicitArgs.addArgument(TemplateArgumentLoc(TypeArg, TypeArgInfo));
>> +
>> + for (unsigned I = 0, N = Lit->getLength(); I != N; ++I) {
>> + Value = Lit->getCodeUnit(I);
>> + TemplateArgument Arg(Context, Value, CharTy);
>> + TemplateArgumentLocInfo ArgInfo(Lit);
>>
>> Using Lit here isn't correct. We should be able to evaluate the
>> expression and get back the same integer value. If we actually need to
>> specify this for some reason, it would be better to synthesize a
>> CharacterLiteral (and point its source location at the relevant character
>> from the source string literal).
>>
>> + ExplicitArgs.addArgument(TemplateArgumentLoc(Arg, ArgInfo));
>> + }
>> + return BuildLiteralOperatorCall(R, OpNameInfo, None,
>> StringTokLocs.back(),
>>
>> StringTokLocs.back() is not really right here. We could have any number
>> of string tokens forming this literal; ideally, we should store all their
>> locations on the UserDefinedLiteral expression.
>>
>> + &ExplicitArgs);
>> + }
>> + default:
>>
>> Please mention LOLR_Error here, rather than using a default: clause. We
>> want a compile error if someone adds another LOLR kind and forgets to
>> handle it here.
>>
>> + return ExprError();
>> + }
>>
>>
>> @@ -2992,11 +3031,11 @@ ExprResult Sema::ActOnNumericConstant(const Token
>> &Tok, Scope *UDLScope) {
>> for (unsigned I = 0, N = Literal.getUDSuffixOffset(); I != N; ++I)
>> {
>> Value = TokSpelling[I];
>> TemplateArgument Arg(Context, Value, Context.CharTy);
>> - TemplateArgumentLocInfo ArgInfo;
>> + TemplateArgumentLocInfo ArgInfo(Lit);
>>
>> Again, this isn't right. Is this change observable somehow? This change
>> seems independent of N3599.
>>
>> - return BuildLiteralOperatorCall(R, OpNameInfo, None,
>> Tok.getLocation(),
>> - &ExplicitArgs);
>> + return BuildLiteralOperatorCall(R, OpNameInfo, None, TokLoc,
>> + &ExplicitArgs);
>>
>> The old indentation here is preferred (we line up to the '(' when
>> breaking a line in function arguments).
>>
>>
>> This needs more tests: in particular, please test that we pass the right
>> types and values as the template arguments, including for the cases where
>> we have an encoding-prefix or a raw string literal.
>>
>>
>> On Thu, Oct 3, 2013 at 12:32 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> Thanks for the patch.
>>>
>>> This proposal was not voted into C++1y, so it should not be controlled
>>> by that language flag, and the compatibility warning is incorrect. Instead,
>>> since GCC trunk has decided to ship this feature, it would make some sense
>>> to treat it as a GNU extension for now. (The response of EWG was more "not
>>> yet" than "no", so there's a good chance we'll have this feature in C++17.)
>>>
>>>
>>> On Wed, Oct 2, 2013 at 2:13 PM, Hristo Venev <mustrumr97 at gmail.com>wrote:
>>>
>>>> Literal operator templates for strings. Tests included.
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131004/aba718c9/attachment.html>
More information about the cfe-commits
mailing list