[cfe-commits] r75808 - in /cfe/trunk: include/clang/AST/ASTContext.h include/clang/AST/Type.h include/clang/Basic/DiagnosticSemaKinds.td include/clang/Frontend/PCHBitCodes.h lib/AST/ASTContext.cpp lib/AST/Type.cpp lib/CodeGen/CodeGenTypes.cpp lib/CodeGen/Mangle.cpp lib/Frontend/PCHReader.cpp lib/Frontend/PCHWriter.cpp lib/Sema/Sema.cpp lib/Sema/SemaExpr.cpp lib/Sema/SemaOverload.cpp test/SemaObjC/comptypes-3.m test/SemaObjC/conditional-expr-3.m

[steve naroff]

On Jul 15, 2009, at 4:45 PM, Fariborz Jahanian wrote:

> Nice. Few comments below.
>

Sure...

> - Fariborz
>
> On Jul 15, 2009, at 11:40 AM, Steve Naroff wrote:
>
>> Author: snaroff
>> Date: Wed Jul 15 13:40:39 2009
>> New Revision: 75808
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=75808&view=rev
>> Log:
>> Implement the ObjC pseudo built-in types as clang "BuiltinType's".  
>> I say pseudo built-in types, since Sema still injects a typedef for  
>> recognition (i.e. they aren't truly built-ins from a parser  
>> perspective).
>>
>> This removes the static data/methods on ObjCObjectPointerType while  
>> preserving the nice API (no need to fiddle with ASTContext:-).
>>
>> This patch also adds Type::isObjCBuiltinType().
>>
>> This should be the last fairly large patch related to recrafting  
>> the ObjC type system. The follow-on patches should be fairly small.
>>

<snip>

>>  bool isObjCIdType(QualType T) const {
>> -    return T == ObjCIdType;
>> +    return T == ObjCIdTypedefType;
>>  }
>>  bool isObjCClassType(QualType T) const {
>> -    return T == ObjCClassType;
>> +    return T == ObjCClassTypedefType;
>
> I am not sure about this and isObjCIdType(). You are comparing  
> against a typedef AST node 'id' and typedef AST node for 'Class'.
> what if 'T' is the builtin type for 'id' or 'Class'? This predicate  
> returns false.
>

All I changed is the name (to avoid confusion with ObjCIdTypedefType/ 
ObjCClassTypedefType).

Nevertheless, you are correct. From a source code perspective, the  
only real 'id' or 'Class' type is the Typedef type (injected by Sema).  
This has always been the case I believe.

The Builtin type is simply a clever mechanism to preserve the nice API  
without resorting to static data. More specifically,  
ASTContext::ObjCBuiltinIdTy and ASTContext::ObjCBuiltinClassTy will  
*only* be referred to by ObjCIdTypedefType/ObjCClassTypedefType.  
Without it, I would have had to move many Type predicates to  
ASTContext (or fiddle with some weird union in ObjCObjectPointerType).  
This seemed like an elegant solution to the problem...especially since  
id/Class are in fact builtin now. The more I played with this  
solution, the more I liked it.


>
>>
>>  }
>>  bool isObjCSelType(QualType T) const {
>>    assert(SelStructType && "isObjCSelType used before 'SEL' type is  
>> built");
>>
>> Modified: cfe/trunk/include/clang/AST/Type.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=75808&r1=75807&r2=75808&view=diff
>>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- cfe/trunk/include/clang/AST/Type.h (original)
>> +++ cfe/trunk/include/clang/AST/Type.h Wed Jul 15 13:40:39 2009
>> @@ -404,6 +404,7 @@
>>  bool isObjCQualifiedIdType() const;           // id<foo>
>>  bool isObjCIdType() const;                    // id
>>  bool isObjCClassType() const;                 // Class
>> +  bool isObjCBuiltinType() const;               // 'id' or 'Class'
>>  bool isTemplateTypeParmType() const;          // C++ template type  
>> parameter
>>  bool isNullPtrType() const;                   // C++0x nullptr_t
>>
>> @@ -592,8 +593,10 @@
>>    Overload,  // This represents the type of an overloaded function  
>> declaration.
>>    Dependent, // This represents the type of a type-dependent  
>> expression.
>>
>> -    UndeducedAuto  // In C++0x, this represents the type of an  
>> auto variable
>> +    UndeducedAuto, // In C++0x, this represents the type of an  
>> auto variable
>>                   // that has not been deduced yet.
>> +    ObjCId,    // This represents the ObjC 'id' type.
>> +    ObjCClass  // This represents the ObjC 'Class' type.
>>  };
>> private:
>>  Kind TypeKind;
>> @@ -1899,7 +1902,7 @@
>> /// Duplicate protocols are removed and protocol list is  
>> canonicalized to be in
>> /// alphabetical order.
>> class ObjCObjectPointerType : public Type, public  
>> llvm::FoldingSetNode {
>> -  QualType PointeeType; // will always point to an interface type.
>> +  QualType PointeeType; // A builin or interface type.
>>
>>  // List of protocols for this protocol conforming object type
>>  // List is sorted on protocol name. No protocol is entered more  
>> than once.
>> @@ -1909,20 +1912,10 @@
>>    Type(ObjCObjectPointer, QualType(), /*Dependent=*/false),
>>    PointeeType(T), Protocols(Protos, Protos+NumP) { }
>>  friend class ASTContext;  // ASTContext creates these.
>> -  friend class ObjCInterfaceType; // To enable 'id' and 'Class'  
>> predicates.
>>
>> -  static ObjCInterfaceType *IdInterfaceT;
>> -  static ObjCInterfaceType *ClassInterfaceT;
>> -  static void setIdInterface(QualType T) {
>> -    IdInterfaceT =  dyn_cast<ObjCInterfaceType>(T.getTypePtr());
>> -  }
>> -  static void setClassInterface(QualType T) {
>> -    ClassInterfaceT =  dyn_cast<ObjCInterfaceType>(T.getTypePtr());
>> -  }
>> -  static ObjCInterfaceType *getIdInterface() { return  
>> IdInterfaceT; }
>> -  static ObjCInterfaceType *getClassInterface() { return  
>> ClassInterfaceT; }
>> public:
>> -  // Get the pointee type. Pointee is required to always be an  
>> interface type.
>> +  // Get the pointee type. Pointee will either be a built-in type  
>> (for 'id' and
>> +  // 'Class') or will be an interface type (for user-defined types).
>>  // Note: Pointee can be a TypedefType whose canonical type is an  
>> interface.
>>  // Example: typedef NSObject T; T *var;
>>  QualType getPointeeType() const { return PointeeType; }
> Name Pointee and comment above is confusing to me. To me Pointee is  
> what a pointer is pointing at.
>
> Here you mean several things as I understand it.
> 1) For 'id' and 'Class' it holds the builtin type for 'id' and  
> 'Class' when their container is ObjCObjectPointerType
> for the two typedefs 'id' and 'Class'.

Yes.

>
>
> 2) For an interface pointer type; such as, "NSObject *" is Pointee  
> for "NSObject" (as the name implies)?
>

Yes.

> 3) For the example  in the comment ( typedef NSObject T; T *var)  
> Pointee referes to "NSObject" right?
>

Nope. Pointee would be a TypedefType whose canonical type is an  
interface. I say this in the "Note:".

Do you think I need to rewrite the comment?

> If my assumptions are correct, then Pointee really means two  
> different things for builtin and  user types.

Sure, but that actually makes more sense than what I previously had.  
For example, the ObjCObjectPointerType::getInterfaceType() method now  
returns 0 for builtin types (which more closely models the existing  
language definition, where there are no formal interfaces for  
'id'/'Class'). The previous implementation would return an internally  
synthesized interface for id/Class with no ivars/methods (which was  
kind of odd). It simplified a very small amount of code internally.

All that said, the good news is the predicates on  
ObjCObjectPointerType abstract you from understanding the two  
different roles (of builtin vs. user defined).  
Type::isObjCBuiltinType() also helps.

Thanks again for reviewing this stuff. Your questions/feedback are  
very helpful.

Let me know if you have any tweaks you'd like me to make (based on my  
responses)...

snaroff