Proposal to fix issue 7715

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

Cheers,
/Manuel

clang-7715.patch (23.7 KB)

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;

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

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

Thanks,
/Manuel

ping

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

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

Hi Doug,

sorry for the delay, I relocated to Mountain View in the meantime :slight_smile: I
applied your suggestions, added a regression test case and updated the
comments.

Cheers,
/Manuel

clang-7715-2.patch (24.8 KB)

Hi Manuel,

Hi Doug,

sorry for the delay, I relocated to Mountain View in the meantime :slight_smile:

Welcome to the area :slight_smile:

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

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

  - Doug