Storage class as written in the source.

Clang Frontend
1

Hello.

Some time ago, Paolo Bolzoni sent to the mailing list a patch meant to enhance VarDecl and FunctionDecl nodes so that they can record the storage class "as written" in the source code (in contrast with the semantic storage class, which could be inherited).

This was the message:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-February/007982.html

The patch was not applied and now is out-of-date.

I have just refreshed it to be a patch against a recent version (r101446, see attached file). It passes all of the clang tests.

It would be nice if someone could have a look on and possibly commit it.

Regards,
Enea Zaffanella.

StorageClassAsWritten.patch (56.5 KB)

2

Hi Enea,

3

Douglas Gregor wrote:

Hi Enea,

Some time ago, Paolo Bolzoni sent to the mailing list a patch meant to enhance VarDecl and FunctionDecl nodes so that they can record the storage class "as written" in the source code (in contrast with the semantic storage class, which could be inherited).

This was the message:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-February/007982.html

The patch was not applied and now is out-of-date.

I have just refreshed it to be a patch against a recent version (r101446, see attached file). It passes all of the clang tests.

It would be nice if someone could have a look on and possibly commit it.

Looks very good. I have a few comments that I'd like to see addressed before I commit.

Thank you for the very quick reply.

[...]

Constructors, destructors, and conversions don't have storage classes in well-formed code, so we shouldn't make the storage-class specifier part of the constructors for the corresponding AST nodes. Just pass FunctionDecl::None down to the CXXMethodDecl constructor in the appropriate place.

Paolo in his message observed that out-of-line definitions of constructors, destructors and conversions can have "extern" as a storage class. The following compiles cleanly on g++:

struct S {
   S();
   ~S();
   operator bool();
};

extern S::S() {}
extern S::~S() {}
extern S::operator bool() { return true; }

Hence, will it be OK if we leave the extra argument in the constructors of the AST nodes above?

(The other comments are finding their way in the revised patch.)

Cheers,
Enea.

4

Enea Zaffanella wrote:
[...]

Hence, will it be OK if we leave the extra argument in the constructors of the AST nodes above?

If the answer to the above question is positive, then here is revised patch.

Cheers,
Enea Zaffanella.

StorageClassAsWritten-2.patch (59.2 KB)

5

Douglas Gregor wrote:

Hi Enea,

Some time ago, Paolo Bolzoni sent to the mailing list a patch meant to enhance VarDecl and FunctionDecl nodes so that they can record the storage class "as written" in the source code (in contrast with the semantic storage class, which could be inherited).

This was the message:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-February/007982.html

The patch was not applied and now is out-of-date.

I have just refreshed it to be a patch against a recent version (r101446, see attached file). It passes all of the clang tests.

It would be nice if someone could have a look on and possibly commit it.

Looks very good. I have a few comments that I'd like to see addressed before I commit.

Thank you for the very quick reply.

[...]

Constructors, destructors, and conversions don't have storage classes in well-formed code, so we shouldn't make the storage-class specifier part of the constructors for the corresponding AST nodes. Just pass FunctionDecl::None down to the CXXMethodDecl constructor in the appropriate place.

Paolo in his message observed that out-of-line definitions of constructors, destructors and conversions can have "extern" as a storage class. The following compiles cleanly on g++:

struct S {
S();
~S();
operator bool();
};

extern S::S() {}
extern S::~S() {}
extern S::operator bool() { return true; }

GCC allows it (but it doesn't have any effect on translation), while EDG rejects it. C++ [class.mem]p5 explicitly calls this ill-formed:

  A member shall not be declared with the extern or register storage-class-specifier.

We should have an Extension/ExtWarn diagnostic about the storage-class-specifier being redundant and then just drop the storage-class-specifier.

Hence, will it be OK if we leave the extra argument in the constructors of the AST nodes above?

I don't think we should model it in the AST because (1) it's not in the language and (2) it has no effect in GCC. Do you disagree?

  - Doug

6

Douglas Gregor wrote:

[...]

struct S {
S();
~S();
operator bool();
};

extern S::S() {}
extern S::~S() {}
extern S::operator bool() { return true; }

GCC allows it (but it doesn't have any effect on translation), while EDG rejects it. C++ [class.mem]p5 explicitly calls this ill-formed:

  A member shall not be declared with the extern or register storage-class-specifier.

We should have an Extension/ExtWarn diagnostic about the storage-class-specifier being redundant and then just drop the storage-class-specifier.

Hence, will it be OK if we leave the extra argument in the constructors of the AST nodes above?

I don't think we should model it in the AST because (1) it's not in the language and (2) it has no effect in GCC. Do you disagree?

  - Doug

Let me try and summarize the state of things, so that you can point out any flaw in my reasoning:

a) any FunctionDecl node should provide the "storage class as written" info for the purposes of source-based applications such as pretty printers, code style checkers and so on;

b) the clang AST hierarchy implies that all nodes encoding C++ methods are FunctionDecl nodes, so that they incur no additional memory cost;

c) since GCC accepts the construct, you (wisely) suggest that clang should allow for the same extension and maybe issue corresponding diagnostic.

The combination of a), b) means that the AST is already able to model this info, whereas c) means that you are willing to add the corresponding logic ... so I cannot see where is the problem.

If the problem is that you do NOT like having an additional argument in the C++ constructor for classes derived from CXXMethodDecl, then we can workaround this while still having this info in the AST.

For instance, we can:

A) let this argument have default value FunctionDecl::None; or

B) completely remove the argument from the CXXConstructorDecl, CXXDestructorDecl and CXXConversionDecl constructors and instead call method FunctionDecl::setStorageClassAsWritten if and when needed.

As for my personal taste, I would go for option A.

Are there other issues we are still missing?

Regards,
Enea.

7

Douglas Gregor wrote:

[...]

struct S {
S();
~S();
operator bool();
};

extern S::S() {}
extern S::~S() {}
extern S::operator bool() { return true; }

GCC allows it (but it doesn't have any effect on translation), while EDG rejects it. C++ [class.mem]p5 explicitly calls this ill-formed:
  A member shall not be declared with the extern or register storage-class-specifier.
We should have an Extension/ExtWarn diagnostic about the storage-class-specifier being redundant and then just drop the storage-class-specifier.

Hence, will it be OK if we leave the extra argument in the constructors of the AST nodes above?

I don't think we should model it in the AST because (1) it's not in the language and (2) it has no effect in GCC. Do you disagree?
  - Doug

Let me try and summarize the state of things, so that you can point out any flaw in my reasoning:

a) any FunctionDecl node should provide the "storage class as written" info for the purposes of source-based applications such as pretty printers, code style checkers and so on;

b) the clang AST hierarchy implies that all nodes encoding C++ methods are FunctionDecl nodes, so that they incur no additional memory cost;

Well, it prevents us from re-using those bits for something else, now or in the future.

c) since GCC accepts the construct, you (wisely) suggest that clang should allow for the same extension and maybe issue corresponding diagnostic.

I'm saying that we should diagnose this violation of the standard and we shouldn't model such ill-formed code in our AST. However, since it's a relatively benign problem (i.e., the presence of such an error would not compromise our AST), I'm okay if the diagnostic is given the ExtWarn/Extension class.

Note that EDG treats this is a hard error, so code that is relying on this extension in GCC is not portable.

The combination of a), b) means that the AST is already able to model this info, whereas c) means that you are willing to add the corresponding logic ... so I cannot see where is the problem.

If the problem is that you do NOT like having an additional argument in the C++ constructor for classes derived from CXXMethodDecl, then we can workaround this while still having this info in the AST.

No, that's not the problem I have. The problem I have is that this appears to be a GCC extension by accident, since it is not documented and has no effect on the translation of the program. The GCC authors probably just forgot to check that part of the standard, like we did. What is the benefit of modeling a GCC accident that is rejected outright by other C++ compilers?

Now, if this extension was *not* an accident and does have some effect on translation, then by all means we should support it and model it in the AST. I haven't seen any evidence that this extension was intentional or has any use.

  - Doug

8

Douglas Gregor wrote:
[...]

The problem I have is that this
appears to be a GCC extension by accident, since it is not documented
and has no effect on the translation of the program. The GCC authors
probably just forgot to check that part of the standard, like we did.
What is the benefit of modeling a GCC accident that is rejected
outright by other C++ compilers?

OK, you convinced me.
Here is the revised patch, modified along your suggestions.

Cheers,
Enea.

StorageClassAsWritten-3.patch (51.8 KB)

9

Thanks! Committed in r101826.

  - Doug