[cfe-commits] r43023 - in /cfe/trunk: Sema/SemaDecl.cpp Sema/SemaExpr.cpp clang.xcodeproj/project.pbxproj

[Chris Lattner]

>>> Bad cast...need to use dyn_cast_or_null. Also changed  
>>> Sema::InitBuiltinVaListType (which had the same bug).
>>
>> Hi Steve,
>>
>> Why are these dyn_cast_or_null?  It seems like none of those cases  
>> allow null pointers?
>>
>
> The second case you forwarded currently needs it (you may be right  
> about the first).

The first:

>>> @@ -154,7 +154,7 @@
>>>    IdentifierInfo *VaIdent = &Context.Idents.get 
>>> ("__builtin_va_list");
>>>    ScopedDecl *VaDecl = LookupScopedDecl(VaIdent,  
>>> Decl::IDNS_Ordinary,
>>>                                            SourceLocation(),  
>>> TUScope);
>>> -  TypedefDecl *VaTypedef = cast<TypedefDecl>(VaDecl);
>>> +  TypedefDecl *VaTypedef = dyn_cast_or_null<TypedefDecl>(VaDecl);
>>>    Context.setBuiltinVaListType(Context.getTypedefType(VaTypedef));
>>>  }

If VaDecl or VaTypedef was null, getTypedefType would crash.  This  
should be cast<>.

The second:

>>> @@ -1896,9 +1896,9 @@
>>>      IdentifierInfo *NSIdent = &Context.Idents.get 
>>> ("NSConstantString");
>>>      ScopedDecl *IFace = LookupScopedDecl(NSIdent,  
>>> Decl::IDNS_Ordinary,
>>>                                           SourceLocation(),  
>>> TUScope);
>>> +    ObjcInterfaceDecl *strIFace =  
>>> dyn_cast_or_null<ObjcInterfaceDecl>(IFace);
>>> +    assert(strIFace && "missing '@interface NSConstantString'");
>>> +    Context.setObjcConstantStringInterface(strIFace);
>>>    }

Here if IFace is null or if it isn't an ObjcInterfaceDecl, the assert  
will trigger.  A similar assert (or crash) would trigger if you use  
cast<>, and cast<> is more efficient for non-assert builds.

> If I don't use dyn_cast_or_null(), the following case will bus  
> error. With dyn_cast_or_null(), we assert (missing '@interface  
> NSConstantString').

I don't see how this is any better: you're trading one crash for  
another.  If you did a check and gracefully handled the problem that  
would be different.

Remember that it should be impossible to trigger asserts.  It seems  
like this should turn into a real check with error recovery.

The third:

>>> +    ObjcInterfaceDecl *strIFace =  
>>> dyn_cast_or_null<ObjcInterfaceDecl>(IFace);
>>> +    assert(strIFace && "missing '@interface NSConstantString'");
>>> +    Context.setObjcConstantStringInterface(strIFace);
>>>    }

Likewise, using cast<> is more efficient in a non-assert build and is  
less code.

-Chris