Fix to the exception specifications type checking

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

Nicola

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

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

core-issue-437.patch (1.51 KB)

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

Bye bye,

Nicola

issue-437.patch (1.62 KB)

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

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

issue-437.patch (2.39 KB)

Thanks! Committed here:

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

  - Doug