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

[Doug Gregor]

On Sun, Apr 6, 2008 at 7:28 PM, Chris Lattner <clattner at apple.com> wrote:
> 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.
>
[snip #2]
>
> 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?

That's reasonable. The updated patch contains a new node
CXXDefaultArgExpr; most of the changes in this patch just deal with
that new node in the statement printer, serialization, code generator,
etc.
The code generator bits were no fun at all, because I ended up having
to duplicate the handling of CXXDefautArgExpr for CGExprComplex,
CGExprConstant, CGExprScalar, and CGExprAgg. I hope there's a better
way to do this, but I didn't dig deep into the CodeGen module before I
convinced myself there wasn't. CXXDefaulArgExpr is a *great* candidate
for a pre-CodeGen lowering phase :)

There's a new test for the CodeGen bits, but I don't know how to
automate the checks we need. If you build it with
-DCLANG_GENERATE_KNOWN_GOOD, it writes out the default arguments
explicitly at the call site; otherwise, it lets the front end fill in
the default arguments. Ideally, we would build this test both ways and
"diff" the byte-code produced from each (that's what I did to verify
the correctness of the CodeGen additions), but I didn't implement that
in the test harness.

[Aside: StmtPrinter did me a great favor by failing to link when I
forgot to add in the CXXDefaultArgExpr case. The serialization code
and CodeGen modules, and perhaps even the isConstantExpr/isLvalue/etc
routines, could all benefit from this kind of built-in safety.]

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

At the moment, this is the only place that does this check. It used to
be in ActOnStartOfFunctionDef, but I've moved it here. Granted, I
don't like that we do this test in Sema... I would much rather handle
(void) parameter lists in the parser, and just put a flag on the decl
to say whether "void" was given to denote an empty parameter list (or,
to be very picky, we could have an EmptyParamListType node that has
the actual "type" the user wrote, if any). Handling it in the parser
makes especially good sense for C++ (and perhaps C89? I don't have the
C standards handy) where the parameter list has to be spelled
(void)... it can't be (T) where T is a typedef of void as I think it
can in C99.

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