[cfe-commits] r113397 - in /cfe/trunk: include/clang/AST/Type.h lib/AST/Type.cpp lib/Sema/SemaType.cpp test/SemaObjC/legacy-objc-types.m test/SemaObjCXX/legacy-objc-types.mm

[John McCall]

On Sep 8, 2010, at 1:08 PM, Fariborz Jahanian wrote:
> Author: fjahanian
> Date: Wed Sep  8 15:08:18 2010
> New Revision: 113397
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=113397&view=rev
> Log:
> Fix a crash when overloading id with objc_object*.
> Radar 8400356.
> 
> 
> Added:
>    cfe/trunk/test/SemaObjC/legacy-objc-types.m
>    cfe/trunk/test/SemaObjCXX/legacy-objc-types.mm
> Modified:
>    cfe/trunk/include/clang/AST/Type.h
>    cfe/trunk/lib/AST/Type.cpp
>    cfe/trunk/lib/Sema/SemaType.cpp
> 
> Modified: cfe/trunk/include/clang/AST/Type.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=113397&r1=113396&r2=113397&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Type.h (original)
> +++ cfe/trunk/include/clang/AST/Type.h Wed Sep  8 15:08:18 2010
> @@ -932,7 +932,9 @@
>   bool isObjCQualifiedClassType() const;        // Class<foo>
>   bool isObjCObjectOrInterfaceType() const;
>   bool isObjCIdType() const;                    // id
> +  bool isLegacyObjCIdType(ASTContext &) const;  // struct_object *
>   bool isObjCClassType() const;                 // Class
> +  bool isLegacyObjCClassType(ASTContext &) const; // struct_class *
>   bool isObjCSelType() const;                 // Class
>   bool isObjCBuiltinType() const;               // 'id' or 'Class'
>   bool isTemplateTypeParmType() const;          // C++ template type parameter
> 
> Modified: cfe/trunk/lib/AST/Type.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=113397&r1=113396&r2=113397&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/Type.cpp (original)
> +++ cfe/trunk/lib/AST/Type.cpp Wed Sep  8 15:08:18 2010
> @@ -470,6 +470,24 @@
>   return false;  
> }
> 
> +bool Type::isLegacyObjCIdType(ASTContext &Ctx) const {
> +  if (const PointerType *PTTo = getAs<PointerType>()) {
> +    QualType PointeeTy = PTTo->getPointeeType();
> +    if (const RecordType *RTy = PointeeTy->getAs<RecordType>())
> +      return RTy->getDecl()->getIdentifier() == &Ctx.Idents.get("objc_object");
> +  }
> +  return false;
> +}
> +
> +bool Type::isLegacyObjCClassType(ASTContext &Ctx) const {
> +  if (const PointerType *PTTo = getAs<PointerType>()) {
> +    QualType PointeeTy = PTTo->getPointeeType();
> +    if (const RecordType *RTy = PointeeTy->getAs<RecordType>())
> +      return RTy->getDecl()->getIdentifier() == &Ctx.Idents.get("objc_class");
> +  }
> +  return false;
> +}
> +
> bool Type::isEnumeralType() const {
>   if (const TagType *TT = dyn_cast<TagType>(CanonicalType))
>     return TT->getDecl()->isEnum();
> 
> Modified: cfe/trunk/lib/Sema/SemaType.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=113397&r1=113396&r2=113397&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaType.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaType.cpp Wed Sep  8 15:08:18 2010
> @@ -1274,8 +1274,12 @@
>               if (BTy->getKind() == BuiltinType::Float)
>                 ArgTy = Context.DoubleTy;
>             }
> +          } else if (getLangOptions().ObjC1) {
> +            if (ArgTy->isLegacyObjCIdType(Context))
> +              ArgTy = Context.getObjCIdType();
> +            else if (ArgTy->isLegacyObjCClassType(Context))
> +              ArgTy = Context.getObjCClassType();
>           }
> -
>           ArgTys.push_back(ArgTy);
>         }


If I understand this patch correctly, there's a crash arising from invalid redeclarations of functions.  If that's true, this is almost certainly a workaround rather than a fix, because it's just making the redeclaration valid.  Please fix the underlying crash rather than working around it like this.

Also, there are several problems with this pattern of recognizing 'struct objc_object' and 'struct objc_class'.
1)  This is a significant change in behavior;  previously we intentionally never recognized these types as being their respective builtin types.  I'm actually ambivalent about that — we should probably recognize this, at the very least to give an error about it — but we shouldn't change the design lightly just to fix a crash.
2)  If we do want to recognize these structs as the builtin types, we should do consistently, not just for parameters, and not just for the exact case of pointers to the type.  The cleanest way to do that is in ConvertDeclSpecToType.
3)  The logic to recognize these structs should respect context;  only structs in global scope are the builtins, i.e. if getDeclContext()->getRedeclContext()->isTranslationUnit() (there might be a convenience function for that somewhere).

John.