On Sat, Oct 5, 2013 at 8:39 AM, Hristo Venev <[email protected]> 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 <[email protected]>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 <[email protected]>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 <[email protected]>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 >>>> <[email protected]>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 <[email protected]>wrote: >>>>> >>>>>> Literal operator templates for strings. Tests included. >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
