[PATCH] C++1y: Implement n3599
Richard Smith
richard at metafoo.co.uk
Mon Oct 7 13:01:10 PDT 2013
On Sat, Oct 5, 2013 at 8:39 AM, Hristo Venev <mustrumr97 at gmail.com> wrote:
> I think it's OK now.
>
Me too =)
I added the new warning to -Wgnu and committed as r192128.
> On Sat, Oct 5, 2013 at 3:12 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> 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/20131007/178843c3/attachment.html>
More information about the cfe-commits
mailing list