A new patch is attached, which:
- has a custom ordering predecate for QualType
- fixes a range bug (where the last handler wasn't checked)
- fixes a crash when the caught exception type is null (i.e. for catch(...))
- adds a testcase
On the map type: I checked http://llvm.org/docs/ProgrammersManual.html#ds_map and none of the other datatypes seem applicable.
Erik.
unreachable-exception-handlers.patch (3.32 KB)
Erik Verbruggen wrote:
A new patch is attached
On the map type: I checked http://llvm.org/docs/ProgrammersManual.html#ds_map and none of the other datatypes seem applicable.
Yes, we really need SmallPtrMap. It would be perfect for this.
Some comments on the patch.
- if (!Handler->getExceptionDecl())
+ if (!Handler->getExceptionDecl() && i < NumHandlers - 1)
return StmtError(Diag(Handler->getLocStart(),
diag::err_early_catch_all));
+
+ // Detect handlers for the same type as an earlier one.
+ const QualType QT = Handler->getCaughtType();
+ if (QT.isNull())
+ continue;
You have some tabs in here, but that's a minor point.
If I remember correctly, Handler->getExceptionDecl() == 0 <=>
Handler->getCaughtType().isNull(). Thus, I think this could be better
written as
if (!Handler->getExceptionDecl()) {
if (i < NumHandlers - 1)
return StmtError(...);
continue;
}
+ const QualType CQT = QT.getTypePtr()->getCanonicalTypeInternal();
Why access the internal function directly? The usual way to get a
canonical type is
Context.getCanonicalType(QT);
Also, I don't like the name QT. It says exactly nothing. Consider
calling the variables Caught and CanonicalCaught (or overwrite Caught
with the canonical form).
+ const QTypeToHandler::const_iterator it = HandlersForTypes.find(CQT);
Since you're inserting when you don't find anything, it's faster (just
one lookup) and IMO more readable to do this:
CXXCatchStmt *&PrevHandler = HandlersForTypes[CQT];
if (PrevHandler) {
// Diags here
} else
PrevHandler = Handler;
Sebastian
Instead of using std::map, please use a SmallVector<pair<>, 8> and then use array_pod_sort (from llvm/ADT/STLExtras.h) to sort it after it is populated. A simple linear scan can then detect dupes.
It looks like some tabs are sneaking in:
+ // Detect handlers for the same type as an earlier one.
+ const QualType QT = Handler->getCaughtType();
+ if (QT.isNull())
+ continue;
You shouldn't use getCanonicalTypeInternal here, you should use ASTContext::getCanonicalType:
+ const QualType CQT = QT.getTypePtr()->getCanonicalTypeInternal();
How does objc treat CV qualifiers on catch types? I don't think that this makes sense, does it?
@catch (NSString *const X) ..
@catch (NSString *volatile X) ..
Also, what about CV qualifiers on the interface, like this:
@catch (const NSString *X) ..
@catch (volatile NSString *X) ..
-Chris
Just a side note, even @catch(NSString *) does not really make sense in fact, at least in 64-bit, as the frameworks require that you do not throw something that does not inherits NSException.
From http://developer.apple.com/releasenotes/Cocoa/RN-ObjectiveC/
The Cocoa frameworks require that all exceptions be instances of NSException or its subclasses. Do not throw objects of other types.