[cfe-dev] Patch & feedback request

[Erik Verbruggen]

Attached is a new patch with:

On Monday, 27 July, 2009, at 05:09PM, "Chris Lattner" <clattner at apple.com> wrote:
>
>On Jul 27, 2009, at 6:14 AM, Erik Verbruggen wrote:
>
>>
>> On Monday, 27 July, 2009, at 01:16PM, "Sebastian Redl" <sebastian.redl at getdesigned.at 
>> > wrote:
>>
>>> 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: unreachable-exception-handlers.patch
Type: application/octet-stream
Size: 5049 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090727/e13209e2/attachment.obj>