Fix to the exception specifications type checking

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Fix to the exception specifications type checking

Nicola Gigante
Hello again!

Douglas pointed me to a FIXME comment in SemaExceptionSpec.cpp that would be interesting to fix. In practice this would allow the last line of test/SemaCXX/exception-spec.cpp to compile (it's commented now).

I don't know very well how are the various clang classes, types etc... (and I'm learning) so I ask some questions:

To fix this bug I need to modify the Sema::CheckSpecifiedExceptionType method to check if the specified type is the class that contains the exception specification itself.
How do I obtain this class name? I think an additional parameter from the caller is required. In this case the caller is Sema::GetTypeForDeclarator. So I think I could get the class from the Declarator object? How?

Thank you :)

Nicola



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

Re: Fix to the exception specifications type checking

Douglas Gregor

On Dec 8, 2009, at 12:25 PM, Nicola Gigante wrote:

> Hello again!
>
> Douglas pointed me to a FIXME comment in SemaExceptionSpec.cpp that  
> would be interesting to fix. In practice this would allow the last  
> line of test/SemaCXX/exception-spec.cpp to compile (it's commented  
> now).
>
> I don't know very well how are the various clang classes, types  
> etc... (and I'm learning) so I ask some questions:
>
> To fix this bug I need to modify the  
> Sema::CheckSpecifiedExceptionType method to check if the specified  
> type is the class that contains the exception specification itself.
> How do I obtain this class name? I think an additional parameter  
> from the caller is required. In this case the caller is  
> Sema::GetTypeForDeclarator. So I think I could get the class from  
> the Declarator object? How?


There's an easier way to tell if a type is being defined. RecordType  
has an isBeingDefined() function that will tell you if that type is  
currently being defined.

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

Re: Fix to the exception specifications type checking

Nicola Gigante

Il giorno 08/dic/2009, alle ore 22.01, Douglas Gregor ha scritto:

>
> There's an easier way to tell if a type is being defined. RecordType has an isBeingDefined() function that will tell you if that type is currently being defined.
>

Thanks! Then the patch is very easy, you find it attached. I've also uncommented that line in the test exception-spec.cpp. At least this is a try, because I'm not sure about a detail. Suppose we have a nested class that contains a method that throws the outer class:

class A {
   class B {
      void f() throw(A);
   };
};

Is this legal? If it is, no problem.

If it isn't, my patch doesn't do the job because isBeingDefined() still returns true.

Nicola


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

core-issue-437.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fix to the exception specifications type checking

Nicola Gigante

Il giorno 08/dic/2009, alle ore 23.37, Sean Hunt ha scritto:

>
> Looking at CWG Issue #437, it should be. It would probably be a good
> idea to add a testcase to make sure that works.
>

Ok. Here is the same patch with a new testcase. I hope it's useful.

Bye bye,

Nicola


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

issue-437.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fix to the exception specifications type checking

Douglas Gregor

On Dec 8, 2009, at 3:05 PM, Nicola Gigante wrote:

>
> Il giorno 08/dic/2009, alle ore 23.37, Sean Hunt ha scritto:
>
>>
>> Looking at CWG Issue #437, it should be. It would probably be a good
>> idea to add a testcase to make sure that works.
>>
>
> Ok. Here is the same patch with a new testcase. I hope it's useful.

A couple comments:

+  // This check deals with core issue 437, when we have a member function of a
+  // class that throws the class itself.

It's nice to quote the actual standard text, so we don't have to refer to some other document when reading through this code, like below:

   // C++ 15.4p2: A type denoted in an exception-specification shall not denote
   //   an incomplete type.

+  if (T->isRecordType() && T->getAs<RecordType>()->isBeingDefined())
+  return false;

Please uses spaces only (no tabs).

There's another call to RequireCompleteType later in the function; you'll need to do a similar check there for pointers  or references to classes that are being defined.

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

Re: Fix to the exception specifications type checking

Nicola Gigante

Il giorno 10/dic/2009, alle ore 02.28, Douglas Gregor ha scritto:

>
> A couple comments:
>
> +  // This check deals with core issue 437, when we have a member function of a
> +  // class that throws the class itself.
>
> It's nice to quote the actual standard text, so we don't have to refer to some other document when reading through this code, like below:
>
Ok, I've quoted the standard in the comment now.

>
> Please uses spaces only (no tabs).
Yes, sorry again, I think there are only spaces now.

>
> There's another call to RequireCompleteType later in the function; you'll need to do a similar check there for pointers  or references to classes that are being defined.

Yes, I think a testcase is needed for that code path, too, so I've added it.

I think everything's ok now.

Thank you,

Nicola


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

issue-437.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fix to the exception specifications type checking

Douglas Gregor

On Dec 10, 2009, at 8:42 AM, Nicola Gigante wrote:

>
> Il giorno 10/dic/2009, alle ore 02.28, Douglas Gregor ha scritto:
>
>>
>> A couple comments:
>>
>> +  // This check deals with core issue 437, when we have a member function of a
>> +  // class that throws the class itself.
>>
>> It's nice to quote the actual standard text, so we don't have to refer to some other document when reading through this code, like below:
>>
> Ok, I've quoted the standard in the comment now.
>
>>
>> Please uses spaces only (no tabs).
> Yes, sorry again, I think there are only spaces now.
>
>>
>> There's another call to RequireCompleteType later in the function; you'll need to do a similar check there for pointers  or references to classes that are being defined.
>
> Yes, I think a testcase is needed for that code path, too, so I've added it.
>
> I think everything's ok now.

Thanks! Committed here:

        http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20091207/025199.html

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