<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Apr 6, 2008, at 3:43 PM, Doug Gregor wrote:</div><blockquote type="cite"><blockquote type="cite">Thanks, it sounds like you fixed the bug for me :-)<br></blockquote><br>I just checked the test cases in 2046 and 2195; both work with this<br>patch, and I've added them as test cases.</blockquote><div><br class="webkit-block-placeholder"></div>Nice.</div><div><br class="webkit-block-placeholder"></div><div><blockquote type="cite"><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">I'm not sure that the handling of 'implicit' typespecs is really the way to</span></blockquote><blockquote type="cite">go for ObjC 'self' and C++ 'this'. [To be clear before I go into it, this<br></blockquote><blockquote type="cite">isn't your fault: this is a pre-existing problem in the code.] The issue is<br></blockquote><blockquote type="cite">that the code currently models 'self' as a ParamDecl, which is reflects its<br></blockquote><blockquote type="cite">common implementation, but doesn't accurately reflect its place in the<br></blockquote><blockquote type="cite">language.<br></blockquote><br>I think it's accurate to say that they are implicit parameters, even<br>in the language sense. I could imagine having an ImplicitParmVarDecl<br>that inherits ParmVarDecl, if that distinction is important enough to<br>warrant a new class. An "isImplicit" flag might be just as good.</blockquote><div><br class="webkit-block-placeholder"></div><div>Ok.</div><div><br class="webkit-block-placeholder"></div><blockquote type="cite"><blockquote type="cite"> I think it would be better for implicitly defined names like self/this to<br></blockquote><blockquote type="cite">be a separate kind of decl (ImplicitDecl?)<br></blockquote><br><blockquote type="cite">which would allow us to get rid<br></blockquote><blockquote type="cite">of the tie in to 'ActOnParamDeclarator', and eliminate the need for<br></blockquote><blockquote type="cite">self/this to be involved at all with the parser-level DeclSpec object.<br></blockquote><br>Looking back at this, my approach here was a bit lame. There are other<br>ways to write CreateImplicitParameter without calling<br>ActOnParamDeclarator directly.</blockquote><div><br class="webkit-block-placeholder"></div>Right. I'm most concerned with not making the DeclSpec object more complex just to handle "self" and friends.</div><div><br class="webkit-block-placeholder"></div><div><br><blockquote type="cite"><blockquote type="cite">Doing this has been on my todo list for a long time, but there was never a<br></blockquote><blockquote type="cite">forcing function. :) I'd prefer to not add 'TST_implicit' though. Would<br></blockquote><blockquote type="cite">you like me to do this change, or would you like to finish this patch and I<br></blockquote><blockquote type="cite">can do it later?<br></blockquote><br>How about this: in the revised patch (attached), I've eliminated<br>TST_implicit by implementing CreateImplicitParameter directly. I think<br>this is a cleaner solution. If you still think you need an<br>ImplicitDecl/ImplicitParmVarDecl, I'll leave that up to you.</blockquote><div><br class="webkit-block-placeholder"></div>Sounds good.</div><div><br><blockquote type="cite"><blockquote type="cite">+++ lib/Sema/SemaExpr.cpp (working copy)<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> Your changes to ActOnCallExpr seem quite reasonable, except that they pass<br></blockquote><blockquote type="cite">in default arguments from the decl into the call when needed. This violates<br></blockquote><blockquote type="cite">the ownership property of the ASTs (that a call, f.e., owns its children<br></blockquote><blockquote type="cite">pointers). Instead of doing this, I think it would make sense to update the<br></blockquote><blockquote type="cite">comments above the CallExpr AST node in Expr.h to say that calls can have<br></blockquote><blockquote type="cite">fewer arguments than the callee if it has default arguments. In addition to<br></blockquote><blockquote type="cite">solving ownership issues, this makes the AST more accurately reflect the<br></blockquote><blockquote type="cite">code the user wrote. Does this seem reasonable?<br></blockquote><br>The problem is that this pushes the handling of default arguments into<br>every consumer of the CallExpr AST, and default arguments won't always<br>be as simple as looking at the corresponding parameter in the function<br>declaration. For example, default arguments in function templates<br>aren't instantiated until they are actually used, so in code like<br>this:<br><br> template<typename T><br> void f(T x, T y = T()) { }<br><br> void g(int x) { f(x); }<br><br>If we just store the 'x' in the CallExpr to f, consumers of that<br>CallExpr will have to look at 'f' to determine that it's a function<br>template specialization, then go back to the function template it was<br>instantiated from to re-instantiate the default argument. That's going<br>to cost compile time (instantiation will never be cheap), but I'm more<br>concerned about the fact that anyone looking at call arguments will<br>need to know about templates.</blockquote><div><br class="webkit-block-placeholder"></div><div>Ok, you're right. This does get ugly fast :)</div><br><blockquote type="cite">Is there, perhaps, a way to clone an expression node, so that the<br>CallExpr could own the clone? The clone could have its source location<br>set to the end of the CallExpr, which could be used as a cheap way to<br>determine that the default argument was used. (e.g.,<br>CallExpr::IsDefaultArg(param_index)), so we would have all of the<br>source information we need.</blockquote><div><br class="webkit-block-placeholder"></div><div>We could do this, but here are two alternative approaches that I like better :).</div><div><br class="webkit-block-placeholder"></div><div>1) we could introduce a new DefaultArgExpr node, which maintains a pointer to a FunctionDecl or ParamDecl (or whatever else is needed). In this case, we'd have an AST for "f(x, DefaultArgExpr(f<int>))". For a client that wants to recursively traverse the AST (e.g. to do code generation) it could just skip into the default expr for the nested decl directly with no other context.</div><div><br class="webkit-block-placeholder"></div><div>2) we could introduce a second sort of DefaultArgExpr node, which is a noop node whose single child is a clone of the default expr. This allows us to capture in the AST the fact that the argument wasn't provided, but clients can just ignore it.</div><div><br></div><div>Of the two, I like #1 the best: it is very explicit, and it doesn't require cloning around ASTs unnecessarily. The down side about it is that clients have to handle one more node type, but I don't think this is a killer. Being an explicit node like this makes it easier to handle, and we keep the invariant that the number of arguments to a call is >= the of formal arguments for the callee. What do you think?</div><div><br></div><blockquote type="cite"><blockquote type="cite"> @@ -768,7 +817,39 @@ Sema::ActOnDeclarator(Scope *S, Declarat<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> + // When we have a prototype for a named function, create the<br></blockquote><blockquote type="cite"> + // parameter declarations for NewFD. C++ needs these early, to<br></blockquote><blockquote type="cite"> + // deal with merging default arguments properly.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> I'm not sure what this is doing. It seems that it could be factored better<br></blockquote><blockquote type="cite">somehow, but I'm not exactly sure what is going on. Can you give an example<br></blockquote><blockquote type="cite">of when this is needed?<br></blockquote><br>I've updated the comment to this:<br><br> // Copy the parameter declarations from the declarator D to<br> // the function declaration NewFD, if they are available.<br><br>Basically, the parameter declarations have all been created for the<br>function declarator (D)... the parser has called ActOnParamDeclarator<br>for each parameter, and put them into the function declarator. This<br>code is copying those parameter declarations into the FunctionDecl for<br>the function. We used to create the parameter declarations at the same<br>time we filled in the FunctionDecl (at definition time), but now we do<br>it as early as possible.<br><blockquote type="cite"><blockquote type="cite"></blockquote></blockquote></blockquote><div><br class="webkit-block-placeholder"></div><div>Ok, I guess my basic objection is the duplication of this logic:</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ if (FTI.NumArgs == 1 && !FTI.isVariadic && FTI.ArgInfo[0].Ident == 0 &&</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ FTI.ArgInfo[0].Param &&</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ !((ParmVarDecl*)FTI.ArgInfo[0].Param)->getType().getCVRQualifiers() &&</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">+ ((ParmVarDecl*)FTI.ArgInfo[0].Param)->getType()->isVoidType()) {</div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;">Can this be factored out into a predicate method and shared with the other code that does it?</span></font></div></div><br><blockquote type="cite"><blockquote type="cite">I'm not sure the right way to go here. I strongly prefer to avoid a global<br></blockquote><blockquote type="cite">'DontAllowParamIdentifiers' flag. Would it be sufficient to do a quick<br></blockquote><blockquote type="cite">recursive walk of the initializer expression looking for ParamDecls?<br></blockquote><br>Yes, we can do a recursive walk. Default arguments are often small, so<br>the cost shouldn't be prohibitive. That'll be a separate patch.<br></blockquote></div><br><div>Sounds great! Thanks again Doug,</div><div><br class="webkit-block-placeholder"></div><div>-Chris</div></body></html>