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

[Chris Lattner]

On Apr 6, 2008, at 10:24 AM, Doug Gregor wrote:
> The attached patch implements support for C++ default arguments, e.g.,
>
>  void g(int x, int y, int z = 0);

Very nice!

> The patch does the following:
>  - Allows the definition of default arguments for function parameters
> (C++ only, of course)
>  - Checks that the default arguments are well-formed w.r.t their
> parameter type
>  - Handles merging of default arguments when merging function  
> declarations
>  - Handles calls to functions that use default arguments

Great!

>  - Introduces parameter names into the "prototype scope" when they
> are parsed, rather than waiting for the function definition.
>
> 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:

int aa(int b, int x[sizeof b]) {}

and C99 allows:

int bb(int sz, int ar[sz][sz])..

Your patch is exactly what I was going to do :)

> For example, when parsing
>
>  void f(int x, int y);
>
> the parameters 'x' and 'y' would never be put into the scope of that
> function prototype. Parameters were only introduced into the function
> scope at definition time, e.g.,
>
>  void f(int x, int y) { /* x and y introduced into scope for the
> definition */ }

Right.

> 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 :-)

> Changes to look out for in this patch:
>  - Action::ActOnParamDeclaratorType has been replaced with
> Action::ActOnParamDeclarator; the latter actually introduces the
> parameter into the current scope
>  - Action::ActOnParamDefaultArgument handles default arguments; the
> C++-specific code for Semi is in the new SemaDeclCXX.cpp

Ok.

>  - The Objective-C Sema code for creating ParmVarDecls has not be
> changed to use Sema::ActOnParamDeclarator, because my understanding of
> Objective-C is far too weak.

No problem.  Others can handle that as soon as the patch goes in.


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.


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.


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


+++ include/clang/AST/Decl.h	(working copy)

this:

+  Expr *getDefaultArg() const { return DefaultArg; }

should be:

+  const Expr *getDefaultArg() const { return DefaultArg; }
+  Expr *getDefaultArg() { return DefaultArg; }




+++ lib/Sema/SemaDeclCXX.cpp	(revision 0)

I'd suggest changing:
+  Expr *DefaultArg = (Expr *)defarg;
to:
+  OwningPtr<Expr> DefaultArg((Expr *)defarg);
and later:
+  Param->setDefaultArg(DefaultArg.take());

to ensure the expr is deleted on error cases.  The code base is  
currently not consistent with this, but I'm slowly trying to move it  
there.



+  // Default arguments are only permitted in C++
+  if (!getLangOptions().CPlusPlus) {
+    Diag(EqualLoc, diag::err_param_default_argument,
+	 DefaultArg->getSourceRange());
+    return;
+ }

Please indent the } one more.  If you want to be fancy, the  
highlighted range could include the equals as well, with something  
like:  SourceRange(EqualLoc, DefaultArg->getLocEnd())   :)


+  // FiXME: CheckSingleAssignmentConstraints has the wrong semantics

FiXME -> FIXME  :)


Two of the methods use brace on next line:

+void Sema::CheckCXXDefaultArguments(FunctionDecl *FD)
+{

one uses brace-on-same line:

+Sema::MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old) {

Please pick one and be consistent :)


I think the code in Sema::CheckCXXDefaultArguments would be more clear  
if the first for-loop was split into two for loops: one that scans for  
the first default argument and the second that verifies the rest of  
the arguments have defaults.  This would eliminate the HasDefaultArg  
bool.

+      if (Param->getIdentifier())
+	Diag(Param->getLocation(),

Please don't use physical tabs in source files, these occur in many  
places.


+    for (unsigned p = 0; p <= LastMissingDefaultArg; ++p) {
+      FD->getParamDecl(p)->setDefaultArg(0);
+    }

Please delete the default arg expr also.

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

Also, please update:

      // If too few arguments are available, don't make the call.

to say something about default args.

+++ lib/Sema/SemaDecl.cpp	(working copy)

+bool Sema::CheckParmsForFunctionDef(FunctionDecl *FD) {
+  bool HasInvalidParm = false;

HasInvalidParm is never changed :)



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


> 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 :)

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


> I ran "make test" within the Clang directory, and there were no
> regressions on x86 Linux. I'm not sure what else I'm supposed to test
> before submitting patches.

That's it!

Thanks a lot Doug,

-Chris