Patch & feedback request

Attached is a new patch with:

I believe, from previous reviews I've seen, that an operator <
should only
be defined if there is a natural ordering. If you simply need an
ordering
for a std::map (which you may want to replace by one of LLVM's own
ADTs, by
the way - another point that comes up often, though it depends on
what you
do with the map), it is, I believe, generally preferred to write a
custom
ordering predicate and supply it as a template parameter.

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.

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.

Fixed

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;

Fixed

You shouldn't use getCanonicalTypeInternal here, you should use
ASTContext::getCanonicalType:

+ const QualType CQT = QT.getTypePtr()->getCanonicalTypeInternal();

Fixed

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

Does ObjC use ActOnCXXTryBlock? And as we're talking about more cases anyway, shouldn't clang warn on:

try {
} catch (Exception e) {}

... saying that an exception should be catched by reference?

Cheers,
Erik.

unreachable-exception-handlers.patch (4.93 KB)

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

Does ObjC use ActOnCXXTryBlock?

No...it uses ActOnObjCAtTryStmt(). CXX usually implies C++-specific.

And as we're talking about more cases anyway, shouldn't clang warn on:

try {
} catch (Exception e) {}

... saying that an exception should be catched by reference?

The code excerpt doesn't give me enough info to respond. Is 'Exception' above an ObjC interface/object? If so, Sema will complain, saying "interface type cannot be statically allocated".

snaroff

steve naroff wrote:

Oh sorry, I didn't look at the code it was patching! Sorry for the confusion, replace @catch with catch :slight_smile:

-Chris