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

[Doug Gregor]

On Sun, Apr 6, 2008 at 3:01 PM, Chris Lattner <clattner at apple.com> wrote:
> On Apr 6, 2008, at 10:24 AM, Doug Gregor wrote:
> > All but the last are relatively straightforward. Up until now,
> > ParseFunctionDeclarator had a simple notion of a function prototype
> > scope, but it was never actually used because parameters were never
> > put into that scope.
> >
>
>  Funny you mention this, I was just working on fixing that for
> http://llvm.org/PR2046 .  C89+ requires this to handle cases like:
[snip]
> > Interestingly, without support for default arguments, we can still use
> > C extensions to get different behavior than we'd expect:
> >
> >  struct S { } s;
> >  void f(int s, typeof(s) t);
> >
> >  void g() {  f(1, 2); }
> >
> > GCC compiles this, because the 's' in typeof(s) refers to the
> > parameter 's', so the parameter 't' is an int.
> > Clang currently rejects this, because name lookup of 's' finds the
> > global struct 's', which isn't compatible with an int.
> > With this patch, Clang does what GCC does, because we always put
> > parameters into prototype scope.
> >
>
>  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.

>  Some thoughts on the patch:
>
>  Now that the decls are being added to scopes, repeated parameter names
> should be found automatically by the normal decl mechanism.  This means you
> can completely remove 'ParamsSoFar' and the #ifdef'd code from
> Parser::ParseFunctionDeclarator.

Oops, I meant to kill that code outright.

>  Also in ParseFunctionDeclarator, I think this:
>
>  +      DeclTy *Param = Actions.ActOnParamDeclarator(CurScope, ParmDecl);
>  +
>  +      // Parse the default argument, if any.
>  +      if (Tok.is(tok::equal)) {
>
>  When I first saw this, I thought it should be: if (Tok.is(tok::equal) &&
> getLang().CPlusPlus).  Please add a comment that says that Sema handles
> rejecting default arguments in C.

Okay.

>  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.

>  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.

> 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.

[snipped stuff below means I agree and have fixed the issue]

>  +++ 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.

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.

>  @@ -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.

> > You'll see a few new FIXMEs in this code that I haven't attacked yet.
> >
>
>  As a general comment, I tend to prefer patches to be broken down into
> distinct unrelated pieces to make development more incremental.  For
> example, it should be possible to split the 'make decls for params' part of
> this patch out and get it committed separately.  Making incremental patches
> makes them easier to review and discuss.  There is no need to do it for this
> patch though, it looks like it is close to ready to going in.  However, I'd
> suggest working getting what you have in first, and finishing these details
> for the next pass :)

Oh, certainly. You only got the whole enchilada in one shot because I
didn't think of using sizeof() to make the parameter-scoping thing
matter in straight C.

> > The most annoying one (for me) is that
> >
> >  void f(int x, int y = x);
> >
> > doesn't actually produce an error. We do name lookup of 'x' in the
> > default argument of 'y' properly, but I don't have a feel for how we
> > should check that parameter names shall not be used in the default
> > argument. It could be done "quickly" by adding a flag in the parser
> > (DontAllowParamIdentifiers) or could be separated from the main flow
> > by passing over the AST for the default arguments within
> > Sema::ActOnParamDefaultArgument.
> >
>
>  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.

  - Doug
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-default-arg-try3.patch
Type: text/x-patch
Size: 50324 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080406/c6d39943/attachment.bin>