[cfe-dev] C++ default arguments patch, rework Parse-Sema interaction for function parameters

[Chris Lattner]

On Apr 6, 2008, at 3:43 PM, Doug Gregor wrote:
>> Thanks, it sounds like you fixed the bug for me :-)
>
> I just checked the test cases in 2046 and  2195; both work with this
> patch, and I've added them as test cases.

Nice.

>> I'm not sure that the handling of 'implicit' typespecs is really  
>> the way to
>> go for ObjC 'self' and C++ 'this'.  [To be clear before I go into  
>> it, this
>> isn't your fault: this is a pre-existing problem in the code.]  The  
>> issue is
>> that the code currently models 'self' as a ParamDecl, which is  
>> reflects its
>> common implementation, but doesn't accurately reflect its place in  
>> the
>> language.
>
> I think it's accurate to say that they are implicit parameters, even
> in the language sense. I could imagine having an ImplicitParmVarDecl
> that inherits ParmVarDecl, if that distinction is important enough to
> warrant a new class. An "isImplicit" flag might be just as good.

Ok.

>> I think it would be better for implicitly defined names like self/ 
>> this to
>> be a separate kind of decl (ImplicitDecl?)
>
>> which would allow us to get rid
>> of the tie in to 'ActOnParamDeclarator', and eliminate the need for
>> self/this to be involved at all with the parser-level DeclSpec  
>> object.
>
> Looking back at this, my approach here was a bit lame. There are other
> ways to write CreateImplicitParameter without calling
> ActOnParamDeclarator directly.

Right. I'm most concerned with not making the DeclSpec object more  
complex just to handle "self" and friends.


>> Doing this has been on my todo list for a long time, but there was  
>> never a
>> forcing function. :)  I'd prefer to not add 'TST_implicit' though.   
>> Would
>> you like me to do this change, or would you like to finish this  
>> patch and I
>> can do it later?
>
> How about this: in the revised patch (attached), I've eliminated
> TST_implicit by implementing CreateImplicitParameter directly. I think
> this is a cleaner solution. If you still think you need an
> ImplicitDecl/ImplicitParmVarDecl, I'll leave that up to you.

Sounds good.

>> +++ lib/Sema/SemaExpr.cpp       (working copy)
>>
>> Your changes to ActOnCallExpr seem quite reasonable, except that  
>> they pass
>> in default arguments from the decl into the call when needed.  This  
>> violates
>> the ownership property of the ASTs (that a call, f.e., owns its  
>> children
>> pointers).  Instead of doing this, I think it would make sense to  
>> update the
>> comments above the CallExpr AST node in Expr.h to say that calls  
>> can have
>> fewer arguments than the callee if it has default arguments.  In  
>> addition to
>> solving ownership issues, this makes the AST more accurately  
>> reflect the
>> code the user wrote.  Does this seem reasonable?
>
> The problem is that this pushes the handling of default arguments into
> every consumer of the CallExpr AST, and default arguments won't always
> be as simple as looking at the corresponding parameter in the function
> declaration. For example, default arguments in function templates
> aren't instantiated until they are actually used, so in code like
> this:
>
>  template<typename T>
>  void f(T x, T y = T()) { }
>
>  void g(int x) { f(x); }
>
> If we just store the 'x' in the CallExpr to f, consumers of that
> CallExpr will have to look at 'f' to determine that it's a function
> template specialization, then go back to the function template it was
> instantiated from to re-instantiate the default argument. That's going
> to cost compile time (instantiation will never be cheap), but I'm more
> concerned about the fact that anyone looking at call arguments will
> need to know about templates.

Ok, you're right.  This does get ugly fast :)

> Is there, perhaps, a way to clone an expression node, so that the
> CallExpr could own the clone? The clone could have its source location
> set to the end of the CallExpr, which could be used as a cheap way to
> determine that the default argument was used.  (e.g.,
> CallExpr::IsDefaultArg(param_index)), so we would have all of the
> source information we need.

We could do this, but here are two alternative approaches that I like  
better :).

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.

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.

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?

>> @@ -768,7 +817,39 @@ Sema::ActOnDeclarator(Scope *S, Declarat
>>
>> +    // When we have a prototype for a named function, create the
>> +    // parameter declarations for NewFD. C++ needs these early, to
>> +    // deal with merging default arguments properly.
>>
>> I'm not sure what this is doing.  It seems that it could be  
>> factored better
>> somehow, but I'm not exactly sure what is going on.  Can you give  
>> an example
>> of when this is needed?
>
> I've updated the comment to this:
>
>    // Copy the parameter declarations from the declarator D to
>    // the function declaration NewFD, if they are available.
>
> Basically, the parameter declarations have all been created for the
> function declarator (D)... the parser has called ActOnParamDeclarator
> for each parameter, and put them into the function declarator. This
> code is copying those parameter declarations into the FunctionDecl for
> the function. We used to create the parameter declarations at the same
> time we filled in the FunctionDecl (at definition time), but now we do
> it as early as possible.
>>

Ok, I guess my basic objection is the duplication of this logic:

+      if (FTI.NumArgs == 1 && !FTI.isVariadic && FTI.ArgInfo[0].Ident  
== 0 &&
+          FTI.ArgInfo[0].Param &&
+          !((ParmVarDecl*)FTI.ArgInfo[0].Param)- 
 >getType().getCVRQualifiers() &&
+          ((ParmVarDecl*)FTI.ArgInfo[0].Param)->getType()- 
 >isVoidType()) {

Can this be factored out into a predicate method and shared with the  
other code that does it?

>> I'm not sure the right way to go here.  I strongly prefer to avoid  
>> a global
>> 'DontAllowParamIdentifiers' flag.  Would it be sufficient to do a  
>> quick
>> recursive walk of the initializer expression looking for ParamDecls?
>
> Yes, we can do a recursive walk. Default arguments are often small, so
> the cost shouldn't be prohibitive. That'll be a separate patch.

Sounds great!  Thanks again Doug,

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080406/61524973/attachment.html>