[cfe-dev] Proposal to fix issue 7715

[Douglas Gregor]

Hi Manuel,

On Oct 11, 2010, at 6:31 PM, Manuel Klimek wrote:

> Hi Doug,
> 
> sorry for the delay, I relocated to Mountain View in the meantime :)

Welcome to the area :)

> I
> applied your suggestions, added a regression test case and updated the
> comments.

Thanks! Committed as r116311. Please go ahead and close PR7715.

	- Doug

> Cheers,
> /Manuel
> 
> On Mon, Sep 27, 2010 at 11:02 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On Sep 22, 2010, at 10:08 AM, Manuel Klimek wrote:
>> 
>>> 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 was looking at the latest draft for the C++0x standard, where the contents of the C++98/03 [class.mem]p2 have moved into C++0x [class.mem]p1, and it now explicitly mentions exception-specifications as well as function bodies, default arguments, and (the C++0x extension) initializers for non-static data members.
>> 
>>> 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.
>> 
>> I was thinking that exception-specifications could be handled at the same time as default arguments, so that we do the initialization in source order.
>> 
>>> 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.
>> 
>> Eventually, we'll have non-static data member initializers, but for now it's just method declarations.
>> 
>>        - Doug
>> 
>> 
> 
> 
> 
> -- 
> Manuel Klimek (http://go/klimek)
> <clang-7715-2.patch>