<div dir="ltr"><div><span style="font-size:13px;font-family:arial,sans-serif">I think I fixed most bugs from the first version of the patch.<br></span>I tried checking if the parameters' type hasQualifiers() on definition and it's always false. Why?</div>
<div>I don't think the type argument should be const-qualified so I'll ask the gcc folks what they think.</div><div>BuildLiteralOperatorCall is given LitEndTok so <span style="font-family:arial,sans-serif;font-size:13px">StringTokLocs.back()</span><span style="font-family:arial,sans-serif;font-size:13px"> should be right.</span></div>
<div>The TokLoc change is because Tok.getLocation() is always called. Now it's only called at one place.</div><div>I also added some raw string/unicode tests and they seem to work as expected.</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, Oct 4, 2013 at 12:06 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>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:</div><div><br></div><div> template<typename T, T...> int &operator""_x(); // #1</div>
<div> float &operator""_x(const char*); // #2</div><div> int &a = "foo"_x; // ok, calls #1</div><div> float &b = 123_x; // ok, calls #2</div><div><br></div><div>It's probably best to repurpose the AllowRawAndTemplate flag into an enum which specifies what form of literal operator you look for.</div>
<div><br></div><div><br></div><div>+ }else if (Params->size() == 2) {</div><div><br></div><div>Space before 'else'</div><div><br></div><div>+ // The second template parameter must be a parameter pack with the<br>
</div><div><div>+ // first template parameter as its type.</div><div>+ if (PmType && PmArgs &&</div><div>+ !PmType->isTemplateParameterPack() &&</div><div>+ PmArgs->isTemplateParameterPack()) {</div>
<div>+ const TemplateTypeParmType *TArgs =</div><div>+ dyn_cast<const TemplateTypeParmType>(PmArgs->getType().getTypePtr());</div><div><br></div><div>You're dropping type qualifiers here. <typename T, const T...> is not an acceptabe template-parameter-list.</div>
<div><br></div><div>+ if(TArgs && TArgs->getDecl() == PmType){</div><div><br></div><div>Space after 'if'.</div><div><br></div><div>+ Valid = true;</div><div>+ if (!getLangOpts().CPlusPlus1y)</div>
<div>+ Diag(FnDecl->getLocation(), diag::warn_string_literal_operator_template);</div><div><br></div><div>This should be an ext_... not a warn_... (and should be a GNU warning not a C++1y warning).</div><div>
<br></div><div>+ }</div><div>+ }</div></div><div><br></div><div><br></div><div><div>@@ -1538,12 +1536,53 @@ Sema::ActOnStringLiteral(const Token *StringToks, unsigned NumStringToks,</div></div><div>+ DeclarationName OpName =<br>
</div><div><div>+ Context.DeclarationNames.getCXXLiteralOperatorName(UDSuffix);</div><div>+ DeclarationNameInfo OpNameInfo(OpName, UDSuffixLoc);</div><div>+ OpNameInfo.setCXXLiteralOperatorNameLoc(UDSuffixLoc);</div>
<div>+</div><div>+ QualType ArgTy[2];</div><div>+ ArgTy[0]=Context.getArrayDecayedType(StrTy);</div><div>+ ArgTy[1]=SizeType;</div><div><br></div><div>Spaces around '='. But please write this using braced initialization.</div>
<div><br></div><div>+</div><div>+ LookupResult R(*this, OpName, UDSuffixLoc, LookupOrdinaryName);</div><div>+ switch (LookupLiteralOperator(UDLScope, R, llvm::makeArrayRef(ArgTy, 2),</div><div>+ /*AllowRawAndTemplate*/true)) {</div>
<div>+</div><div>+ case LOLR_Cooked:</div><div>+ case LOLR_Raw: {</div><div><br></div><div>llvm_unreachable on the Raw case; that can't happen for a string.</div><div><br></div><div>+ llvm::APInt Len(Context.getIntWidth(SizeType), Literal.GetNumStringChars());</div>
<div>+ IntegerLiteral *LenArg = IntegerLiteral::Create(Context, Len, SizeType,</div><div>+ StringTokLocs[0]);</div><div>+ Expr *Args[] = { Lit, LenArg };</div><div>
+</div><div>+ return BuildLiteralOperatorCall(R, OpNameInfo, Args, StringTokLocs.back());</div><div>+ }</div><div>+</div><div>+ case LOLR_Template: {</div><div>+ TemplateArgumentListInfo ExplicitArgs;</div><div>+</div>
<div>+ unsigned CharBits = Context.getIntWidth(CharTy);</div><div>+ bool CharIsUnsigned = CharTy->isUnsignedIntegerType();</div><div>+ llvm::APSInt Value(CharBits, CharIsUnsigned);</div><div>+</div><div>+ TemplateArgument TypeArg(CharTy);</div>
<div><br></div><div>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 =)</div>
<div><br></div><div>+ TemplateArgumentLocInfo TypeArgInfo(Context.getTrivialTypeSourceInfo(CharTy));</div><div>+ ExplicitArgs.addArgument(TemplateArgumentLoc(TypeArg, TypeArgInfo));</div><div>+</div><div>+ for (unsigned I = 0, N = Lit->getLength(); I != N; ++I) {</div>
<div>+ Value = Lit->getCodeUnit(I);</div><div>+ TemplateArgument Arg(Context, Value, CharTy);</div><div>+ TemplateArgumentLocInfo ArgInfo(Lit);</div><div><br></div><div>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).</div>
<div><br></div><div>+ ExplicitArgs.addArgument(TemplateArgumentLoc(Arg, ArgInfo));</div><div>+ }</div><div>+ return BuildLiteralOperatorCall(R, OpNameInfo, None, StringTokLocs.back(),</div><div><br></div><div>
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.</div>
<div><br></div><div>+ &ExplicitArgs);</div><div>+ }</div><div>+ default:</div><div><br></div><div>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.</div>
<div><br></div><div>+ return ExprError();</div><div>+ }</div></div><div><br></div><div><br></div><div><div>@@ -2992,11 +3031,11 @@ ExprResult Sema::ActOnNumericConstant(const Token &Tok, Scope *UDLScope) {</div><div>
for (unsigned I = 0, N = Literal.getUDSuffixOffset(); I != N; ++I) {</div><div> Value = TokSpelling[I];</div><div> TemplateArgument Arg(Context, Value, Context.CharTy);</div><div>- TemplateArgumentLocInfo ArgInfo;</div>
<div>+ TemplateArgumentLocInfo ArgInfo(Lit);</div><div><br></div></div><div>Again, this isn't right. Is this change observable somehow? This change seems independent of N3599.</div><div><br></div><div><div>- return BuildLiteralOperatorCall(R, OpNameInfo, None, Tok.getLocation(),</div>
<div>- &ExplicitArgs);</div><div>+ return BuildLiteralOperatorCall(R, OpNameInfo, None, TokLoc,</div><div>+ &ExplicitArgs);</div></div><div><br></div><div>The old indentation here is preferred (we line up to the '(' when breaking a line in function arguments).</div>
<div><br></div><div><br></div><div>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.</div>
</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 3, 2013 at 12:32 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks for the patch.<div><br></div><div>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.)</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Oct 2, 2013 at 2:13 PM, Hristo Venev <span dir="ltr"><<a href="mailto:mustrumr97@gmail.com" target="_blank">mustrumr97@gmail.com</a>></span> wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div dir="ltr">Literal operator templates for strings. Tests included.</div>
<br></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>