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

[Doug Gregor]

On Tue, Apr 8, 2008 at 1:32 AM, Chris Lattner <clattner at apple.com> wrote:
> On Apr 7, 2008, at 6:11 AM, Doug Gregor wrote:
> > 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.
> >
>
>  Looks great, I like this approach.  Instead of dwelling on details and
> forcing you to keep the patch up to date, I committed it here (after
> resolving a few conflicts that came up in the short time since you sent it
> out!):
>
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005079.html

Thanks! I'm following this up with another patch (attached) that fixes
the remaining nits below. It also implements proper checking for
ill-formed code like

  void f(int x, int y = x);

and, when in non-C99 mode, warns about this as an extension:

  typedef void T;
  void f(T);

>  I'm not sure about that.  clang tracks typedefs (and lack thereof) well
> enough that Sema can decide whether it was spelled "(void)" or "(T)".  Also,
> we do have to handle the "(T)" case for C99.  It seems to me that handling
> this in the parser would require handling it in both the parser and Sema.

Okay. Implemented in Sema, now.

>  +    virtual SourceRange getSourceRange() const {
>  +      return Param->getDefaultArg()->getSourceRange();
>  +    }
>
>  I don't think that this is right.  The source range of the
> CXXDefaultArgExpr should be empty, not the source range of the input.  The
> default argument doesn't occur in the source, so it shouldn't have a
> location.

Okay.

>  +++ lib/AST/ExprCXX.cpp (working copy)
>
>  +// CXXDefaultArgExpr
>  +Stmt::child_iterator CXXDefaultArgExpr::child_begin() {
>  +  return reinterpret_cast<Stmt**>(Param->getDefaultArg());
>  +}
>
>  This is subtly wrong.  The child iteration stuff should walk the expression
> tree in the current function, it shouldn't "jump the gap here".  I suspect
> that -ast-dump will have problems with CXXDefaultArgExpr's due to this.  I'd
> just have CXXDefaultArgExpr just return an empty child range.

Okay, understood.

>
>   +void
>  +Sema::ActOnParamDefaultArgument(DeclTy *param, SourceLocation EqualLoc,
>  +                                ExprTy *defarg) {
>  ...
>  +    // Some default arguments were missing. Clear out all of the
>  +    // default arguments up to (and including) the last missing
>  +    // default argument, so that we leave the function parameters
>  +    // in a semantically valid state.
>  +    for (p = 0; p <= LastMissingDefaultArg; ++p) {
>  +      ParmVarDecl *Param = FD->getParamDecl(p);
>  +      if (Param->getDefaultArg()) {
>  +        delete Param->getDefaultArg();
>  +        Param->setDefaultArg(0);
>  +      }
>  +    }
>
>  Should these ParmVarDecl's be marked as erroneous?  This might be a good
> idea if there are secondary errors that can occur because they are missing
> default initializers.  I can't think of any specific examples off the top of
> my head though, your call.

Secondary errors could occur as the result of a function call that
relies on those default arguments, but there isn't much we can do
about that. I say we just leave the parameters alone: other than not
having default arguments any more, there isn't anything wrong with
them. (Most importantly: the AST is still in a perfectly consistent
state after removing default arguments)

>  +++ lib/Sema/SemaExpr.cpp       (working copy)
>
>    // Make the call expr early, before semantic checks.  This guarantees
> cleanup
>    // of arguments and function on error.
>  -  llvm::OwningPtr<CallExpr> TheCall(new CallExpr(Fn, Args, NumArgs,
>  +  if (getLangOptions().CPlusPlus && FDecl && NumArgs <
> FDecl->getNumParams())
>  +    NumArgsPassed = FDecl->getNumParams();
>  +  llvm::OwningPtr<CallExpr> TheCall(new CallExpr(Fn, Args, NumArgsPassed,
>                                                   Context.BoolTy,
> RParenLoc));
>
>  I don't think this is safe.  The CallExpr ctor reads the range
> [Args,Args+NumArgsPassed), so changing NumArgsPassed in this way makes it
> potentially read beyond the end of the args array.

/me smacks forehead, fixes bug

>
>  +    // If too few arguments are available (and we don't have default
>  +    // arguments for the remaining parameters), don't make the call.
>  +    if (NumArgs < NumArgsInProto) {
>  +      if (getLangOptions().CPlusPlus &&
>  +          FDecl &&
>  +          FDecl->getParamDecl(NumArgs)->getDefaultArg()) {
>  +        // Use default arguments for missing arguments
>  +        NumArgsToCheck = NumArgsInProto;
>  +      } else
>  +        return Diag(RParenLoc, diag::err_typecheck_call_too_few_args,
>  +                    Fn->getSourceRange());
>  +    }
>
>  This logic is reasonable, but what do you think about adding a new method
> to FunctionDecl, something like "unsigned getMinRequiredArguments() const"
> or something like that, which would scan the argument list for default
> arguments.  I think this is a generally useful method, and could simplify
> this down to:
>
>  if (NumArgs < NumArgsInProto) {
>   if (FDecl && NumArgs >= FDecl->getMinRequiredArguments())
>      ...
>
>  which would be simpler :).  It could also be used to simplify the logic
> above.

Sure, although I'm going to have to point out that we're replacing a
constant-time check (is a particular pointer non-NULL) with a loop
that spins through the parameter list :)

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