[PATCH] C++1y: Implement n3599
Hristo Venev
mustrumr97 at gmail.com
Sat Oct 5 08:39:31 PDT 2013
I think it's OK now.
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/20131005/1706a75b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-C-1y-Implement-n3599-v3.patch
Type: application/octet-stream
Size: 22291 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131005/1706a75b/attachment.obj>
More information about the cfe-commits
mailing list