[cfe-dev] Proposal to fix issue 7715
Manuel Klimek
klimek at google.com
Wed Sep 22 10:08:32 PDT 2010
On Mon, Sep 20, 2010 at 8:28 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> 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.
I trust your gut-feeling then ;)
> 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()".
While trying to comment that part I got pretty confused by what you
write. In the C++ standard I didn't find anything in [class.mem]p1
that mentions function bodies (am I reading the wrong version?), but I
find that in [class.mem]p2 it talks about "function bodies, default
arguments and constructor ctor-initializers", where I'd expect
ctor-initializers to be basically handled the same as function bodies.
Where do exception specifications fall into all this?
I called the methods ParseLexedDefaultArguments and
ParseLexedMethodDefs as the function bodies must be parsed in a phase
after all default arguments were parsed, since as far as I understand
it the default arguments can always be used in the function bodies.
I'd assume that anything else would be basically handled in a
different phase, too.
The interface is called LateParsedDeclaration (perhaps wanting a
better name), as all the late parsed stuff is always combined with a
method declaration, as far as I understood it.
So overall I'm pretty confused and would appreciate some enlightenment ;)
Thanks,
/Manuel
>
> 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
--
Manuel Klimek (http://go/klimek)
More information about the cfe-dev
mailing list