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

[Chris Lattner]

On Apr 8, 2008, at 2:55 PM, Doug Gregor wrote:
>> 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);

Nice.

> and, when in non-C99 mode, warns about this as an extension:
>
>  typedef void T;
>  void f(T);

I tweaked this for c89.

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

Sounds good.  Keeping the AST consistent on errors is a high priority,  
thanks :)

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

You're definitely right.  If it ever becomes a problem we can  
reevaluate it :).

The patch looks great except some nit-picky stuff, I applied your  
patch here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005098.html

The only change I made was to fold a few regtests together into one  
file.  While our current testsuite doesn't have much specific  
structure, for language features I'd generally prefer to have one file  
for each "feature" and have multiple tests within that file.  This  
keeps the # files down, which speeds up testing and will hopefully  
keep the suite reasonable.  This is only possible for stuff that uses - 
verify of course.

Here is the C89 tweak:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005099.html

Here are the nit-picks:

+++ include/clang/AST/ExprCXX.h	(working copy)
@@ -149,7 +149,9 @@ namespace clang {
      Expr *getExpr() { return Param->getDefaultArg(); }

      virtual SourceRange getSourceRange() const {
-      return Param->getDefaultArg()->getSourceRange();
+      // Default argument expressions have no represntation in the
+      // source, so they have an empty source range.
+      return SourceRange();

spello represntation

+++ include/clang/AST/Decl.h	(working copy)
@@ -352,6 +352,7 @@ public:
      return ParamInfo[i];
    }
    void setParams(ParmVarDecl **NewParamInfo, unsigned NumParams);
+  unsigned getMinRequiredArguments() const;


Please add a copy of your doxygen comment from the cpp file.  Please  
mention "C++ default arguments".


+++ lib/Sema/SemaExpr.cpp	(working copy)
@@ -637,11 +634,10 @@ ActOnCallExpr(ExprTy *fn, SourceLocation
      // 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()) {
+      if (FDecl && NumArgs >= FDecl->getMinRequiredArguments()) {
          // Use default arguments for missing arguments
          NumArgsToCheck = NumArgsInProto;
+        TheCall->setNumArgs(NumArgsInProto);
        } else

Nice!

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

+/// CheckDefaultArgumentVisitor - Traverses the default argument of a
+/// parameter to determine whether it contains any ill-formed
+/// subexpressions. For example, this will diagnose the use of local
+/// variables or parameters within the default argument expression.
+class VISIBILITY_HIDDEN CheckDefaultArgumentVisitor

Please wrap this class in an anonymous namespace.  Is there a spec  
citation that you can give for this?  Maybe C++ [dcl.fct.default] is  
enough.


+bool CheckDefaultArgumentVisitor::VisitExpr(Expr *Node) {
+  bool IsInvalid = false;
+  for (Stmt::child_iterator first = Node->child_begin(),
+                            last = Node->child_end();
+       first != last; ++first)
+    IsInvalid |= Visit(*first);
+
+  return IsInvalid;
+}

Does it make sense to report multiple diagnostics for a malformed  
default?  If not, you might want to change it to:

+bool CheckDefaultArgumentVisitor::VisitExpr(Expr *Node) {
+  for (Stmt::child_iterator first = Node->child_begin(),
+                            last = Node->child_end();
+       first != last; ++first)
+    if (Visit(*first)) return true;
+  return false;
+}

This would affect something like "void foo(int i, int j, int k = i 
+j)".  This might be too contrived to matter.

+    //   Default arguments are evaluated each time the function is
+    //   called. The order of evaluation of function arguments is
+    //   unspecified. Consequently, parameters of a function shall not
+    //   be used in default argument expressions, even if they are not
+    //   evaluated. Parameters of a function declared before a default
+    //   argument expression are in scope and can hide namespace and
+    //   class member names.
+    return S->Diag(DRE->getSourceRange().getBegin(),
+                   diag::err_param_default_argument_references_param,
+                   Param->getName());

It's a very minor thing, but it would be nice to highlight the  
sourcerange for the entire default arg expr as well, giving something  
like:

void foo(int i, int j, int k = i+j)
                                ^~~

This would require capturing the root of the default arg expr in the  
CheckDefaultArgumentVisitor, but that doesn't seem too evil :)

+    // C++ [dcl.fct.default]p7
+    //   Local variables shall not be used in default argument
+    //   expressions.
+    return S->Diag(DRE->getSourceRange().getBegin(),
+                   diag::err_param_default_argument_references_local,
+                   BlockVar->getName());

Note that this will also exclude local static variables and local  
extern variables.  For example:

void foo() {
   extern int x;
   void bar(int z = x);
}

Is that expected?


Nice work Doug,

-Chris