[cfe-dev] Patch & feedback request

[Chris Lattner]

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.

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