<div dir="ltr">I think it's OK now.<div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Oct 5, 2013 at 3:12 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>This looks really good. Just a few more minor things and this will be ready to commit.</div><div class="im">
<div><br></div><div><br></div><div>+ // The second template parameter must be a parameter pack with the</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><div>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):</div>
<div><br></div><div>template<typename T> using X = T;</div><div>template<typename T, X<T>... Chars> void operator""_x();</div><div><br></div><div><br></div><div><div>+ if (TArgs && TArgs->getDecl() == PmType){</div>
</div><div><br></div><div>Space before {, please.</div><div><br></div><div><br></div><div><div>+ if (ActiveTemplateInstantiations.empty() && !getLangOpts().GNUMode)</div><div>+ Diag(FnDecl->getLocation(), diag::ext_string_literal_operator_template);</div>
</div><div><br></div><div>Please remove the GNUMode check here (we want to be able to warn on this extension even in -std=gnu++1y mode).</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">
<div class="gmail_extra">+ if (FunctionTemplateDecl *FD = dyn_cast<FunctionTemplateDecl>(D)) {</div><div class="gmail_extra">+ TemplateParameterList *Params=FD->getTemplateParameters();</div><div class="gmail_extra">
+ if (Params->size() == 1)</div><div class="gmail_extra">+ IsTemplate=true;</div><div><br></div><div>Please add spaces around the '=' here (2x).</div><div><br></div><br><div class="gmail_quote"><div class="im">
On Fri, Oct 4, 2013 at 11:39 AM, Hristo Venev <span dir="ltr"><<a href="mailto:mustrumr97@gmail.com" target="_blank">mustrumr97@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><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></blockquote><div><br></div></div><div>Oh, sorry. The qualifiers are dropped when forming the parameter's type; we don't need to check for them.</div>
<div class="im">
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<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></blockquote><div><br></div></div><div>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.</div>
<div class="im">
<div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<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></blockquote>
<div><br></div></div><div>Thanks!</div><div><div class="h5"><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><div class="gmail_extra"><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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><div><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>