Storage class as written in the source.

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Storage class as written in the source.

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

StorageClassAsWritten.patch (57K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Douglas Gregor
Hi Enea,

On Apr 16, 2010, at 12:27 PM, Enea Zaffanella wrote:

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

Index: include/clang/AST/DeclCXX.h
===================================================================
--- include/clang/AST/DeclCXX.h (revision 101446)
+++ include/clang/AST/DeclCXX.h (working copy)
@@ -1186,8 +1187,9 @@
   CXXConstructorDecl(CXXRecordDecl *RD, SourceLocation L,
                      DeclarationName N, QualType T, TypeSourceInfo *TInfo,
                      bool isExplicitSpecified, bool isInline,
-                     bool isImplicitlyDeclared)
-    : CXXMethodDecl(CXXConstructor, RD, L, N, T, TInfo, false, isInline),
+                     bool isImplicitlyDeclared, StorageClass SCAsWritten)
+    : CXXMethodDecl(CXXConstructor, RD, L, N, T, TInfo, false, SCAsWritten,
+                    isInline),
       IsExplicitSpecified(isExplicitSpecified), ImplicitlyDefined(false),
       BaseOrMemberInitializers(0), NumBaseOrMemberInitializers(0) {
     setImplicit(isImplicitlyDeclared);
@@ -1199,7 +1201,8 @@
                                     SourceLocation L, DeclarationName N,
                                     QualType T, TypeSourceInfo *TInfo,
                                     bool isExplicit,
-                                    bool isInline, bool isImplicitlyDeclared);
+                                    bool isInline, bool isImplicitlyDeclared,
+                                    StorageClass SCAsWritten);
 
   /// isExplicitSpecified - Whether this constructor declaration has the
   /// 'explicit' keyword specified.
@@ -1329,8 +1332,10 @@
   
   CXXDestructorDecl(CXXRecordDecl *RD, SourceLocation L,
                     DeclarationName N, QualType T,
-                    bool isInline, bool isImplicitlyDeclared)
-    : CXXMethodDecl(CXXDestructor, RD, L, N, T, /*TInfo=*/0, false, isInline),
+                    bool isInline, bool isImplicitlyDeclared,
+                    StorageClass SCAsWritten)
+    : CXXMethodDecl(CXXDestructor, RD, L, N, T, /*TInfo=*/0, false,
+                    SCAsWritten, isInline),
       ImplicitlyDefined(false), OperatorDelete(0) {
     setImplicit(isImplicitlyDeclared);
   }
@@ -1339,7 +1344,8 @@
   static CXXDestructorDecl *Create(ASTContext &C, CXXRecordDecl *RD,
                                    SourceLocation L, DeclarationName N,
                                    QualType T, bool isInline,
-                                   bool isImplicitlyDeclared);
+                                   bool isImplicitlyDeclared,
+                                   StorageClass SCAsWritten);
 
   /// isImplicitlyDefined - Whether this destructor was implicitly
   /// defined. If false, then this destructor was defined by the
@@ -1385,15 +1391,18 @@
 
   CXXConversionDecl(CXXRecordDecl *RD, SourceLocation L,
                     DeclarationName N, QualType T, TypeSourceInfo *TInfo,
-                    bool isInline, bool isExplicitSpecified)
-    : CXXMethodDecl(CXXConversion, RD, L, N, T, TInfo, false, isInline),
+                    bool isInline, bool isExplicitSpecified,
+                    StorageClass SCAsWritten)
+    : CXXMethodDecl(CXXConversion, RD, L, N, T, TInfo, false, SCAsWritten,
+                    isInline),
       IsExplicitSpecified(isExplicitSpecified) { }
 
 public:
   static CXXConversionDecl *Create(ASTContext &C, CXXRecordDecl *RD,
                                    SourceLocation L, DeclarationName N,
                                    QualType T, TypeSourceInfo *TInfo,
-                                   bool isInline, bool isExplicit);
+                                   bool isInline, bool isExplicit,
+                                   StorageClass SCAsWritten);
 
   /// IsExplicitSpecified - Whether this conversion function declaration is
   /// marked "explicit", meaning that it can only be applied when the user

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.

Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp (revision 101446)
+++ lib/Sema/SemaDecl.cpp (working copy)
@@ -1729,12 +1732,29 @@
       SC = VarDecl::None;
       break;
     }
+    VarDecl::StorageClass SCAsWritten;
+    switch (DS.getStorageClassSpecAsWritten()) {
+      default: /* Uncorrect program: we mark the sintatic
+                  storage class as none. */
+      case DeclSpec::SCS_unspecified:
+        SCAsWritten = VarDecl::None; break;
+      case DeclSpec::SCS_extern:
+        SCAsWritten = VarDecl::Extern; break;
+      case DeclSpec::SCS_static:
+        SCAsWritten = VarDecl::Static; break;
+      case DeclSpec::SCS_auto:
+        SCAsWritten = VarDecl::Auto; break;
+      case DeclSpec::SCS_register:
+        SCAsWritten = VarDecl::Register; break;
+      case DeclSpec::SCS_private_extern:
+        SCAsWritten = VarDecl::PrivateExtern; break;
+    }

We don't need the default case here. It's better to write out all of the cases.

@@ -2350,6 +2370,23 @@
     SC = VarDecl::None;
     break;
   }
+  VarDecl::StorageClass SCAsWritten;
+  switch (D.getDeclSpec().getStorageClassSpecAsWritten()) {
+    default: /* Uncorrect program: we mark the sintatic
+                storage class as none. */
+    case DeclSpec::SCS_unspecified:
+      SCAsWritten = VarDecl::None; break;
+    case DeclSpec::SCS_extern:
+      SCAsWritten = VarDecl::Extern; break;
+    case DeclSpec::SCS_static:
+      SCAsWritten = VarDecl::Static; break;
+    case DeclSpec::SCS_auto:
+      SCAsWritten = VarDecl::Auto; break;
+    case DeclSpec::SCS_register:
+      SCAsWritten = VarDecl::Register; break;
+    case DeclSpec::SCS_private_extern:
+      SCAsWritten = VarDecl::PrivateExtern; break;
+  }

Copy-paste from above? Please factor this into its own utility routine.

@@ -2801,7 +2838,21 @@
   bool isInline = D.getDeclSpec().isInlineSpecified();
   bool isVirtual = D.getDeclSpec().isVirtualSpecified();
   bool isExplicit = D.getDeclSpec().isExplicitSpecified();
+  FunctionDecl::StorageClass SCAsWritten;
+  switch (D.getDeclSpec().getStorageClassSpecAsWritten()) {
+    default: /* Uncorrect program: we mark the sintatic
+                storage class as none. */
+    case DeclSpec::SCS_unspecified:
+      SCAsWritten = FunctionDecl::None; break;
+    case DeclSpec::SCS_extern:
+      SCAsWritten = FunctionDecl::Extern; break;
+    case DeclSpec::SCS_static:
+      SCAsWritten = FunctionDecl::Static; break;
+    case DeclSpec::SCS_private_extern:
+      SCAsWritten = FunctionDecl::PrivateExtern; break;
+  }

We don't want the default case here, either.

        - Doug


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Enea Zaffanella
Douglas Gregor wrote:

> Hi Enea,
>
> On Apr 16, 2010, at 12:27 PM, Enea Zaffanella wrote:
>> 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.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

StorageClassAsWritten-2.patch (60K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Douglas Gregor
In reply to this post by Enea Zaffanella

On Apr 16, 2010, at 11:48 PM, Enea Zaffanella wrote:

> Douglas Gregor wrote:
>> Hi Enea,
>>
>> On Apr 16, 2010, at 12:27 PM, Enea Zaffanella wrote:
>>> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Enea Zaffanella
Douglas Gregor wrote:
> On Apr 16, 2010, at 11:48 PM, Enea Zaffanella 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.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Douglas Gregor

On Apr 17, 2010, at 10:02 AM, Enea Zaffanella wrote:

> Douglas Gregor wrote:
>> On Apr 16, 2010, at 11:48 PM, Enea Zaffanella 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

StorageClassAsWritten-3.patch (52K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Storage class as written in the source.

Douglas Gregor

On Apr 18, 2010, at 11:54 AM, Enea Zaffanella wrote:

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

Thanks! Committed in r101826.

        - Doug
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev