[cfe-dev] Proposal to fix issue 7715

[Douglas Gregor]

On Sep 14, 2010, at 7:39 AM, Manuel Klimek wrote:

> Hi,
> 
> I came up with a solution to http://llvm.org/bugs/show_bug.cgi?id=7715
> (clang++ gives wrong error about default argument in kdatetime.h), as
> talked about on IRC. It was a little more complicated than
> anticipated, as we need to keep the stack of scopes correct.
> 
> I would like to get some feedback on the general direction /
> implementation of this patch. If we agree this is the way to go, I'd
> comment it more thoroughly and add some more regression tests.

I like the general direction; there are a few comments below. Obviously, more comments and regression tests would be greatly appreciated!

> I tried
> to keep the diff as small as possible, but my gut feeling would also
> lead me to put more stuff into the classes as opposed to having
> everything as methods on Parser. Not sure though.

I'd rather keep the parsing and Sema-invoking code in the parser itself, although I admit it's mostly an aesthetic argument and it's not a strong preference.

A few small comments:

diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h
index a7dab1e..422adce 100644
--- a/include/clang/Parse/Parser.h
+++ b/include/clang/Parse/Parser.h
@@ -552,7 +552,30 @@ private:
   //===--------------------------------------------------------------------===//
   // Lexing and parsing of C++ inline methods.
 
-  struct LexedMethod {
+  struct ParsingClass;
+
+  class LateParsedDeclaration {
+  public:
+    virtual ~LateParsedDeclaration();
+    virtual void ParseLexedDefaultArguments();
+    virtual void ParseLexedMethodDefs();
+  };

I think the name ParseLexedDefaultArguments() is misleading, because default arguments aren't the only kind of thing that is delayed. C++ [class.mem]p1 also mentions function bodies (which we handle separately) and exception-specifications (which we don't handle)... the latter is why I'd prefer to keep a more generic name like "ParseLexedMethodDeclarations()".

In Parser::ParseLexedMethodDeclaration(), we have:

+      // Consume the '='.
+      assert(Tok.is(tok::equal) && "Default argument not starting with '='");
+      SourceLocation EqualLoc = ConsumeToken();
+
+      ExprResult DefArgResult(ParseAssignmentExpression());
+      if (DefArgResult.isInvalid())
+        Actions.ActOnParamDefaultArgumentError(LM.DefaultArgs[I].Param);
+      else {
+        if (Tok.is(tok::cxx_defaultarg_end))
+          ConsumeToken();
+        else
+          Diag(Tok.getLocation(), diag::err_default_arg_unparsed);
+        Actions.ActOnParamDefaultArgument(LM.DefaultArgs[I].Param, EqualLoc,
+                                          DefArgResult.take());
+      }

There is (recently-added) code that enters the appropriate expression-evaluation context for default arguments, like this:

        // The argument isn't actually potentially evaluated unless it is 
        // used.
        EnterExpressionEvaluationContext Eval(Actions,
                                              Sema::PotentiallyEvaluatedIfUsed);

Please make sure that gets into your patch.

Thanks for tackling this! I look forward to your next revision; commenting/testing aside, it looks very close to being ready to commit.

	- Doug