[cfe-commits] r49058 - in /cfe/trunk: include/clang/AST/Decl.h include/clang/AST/DeclObjC.h include/clang/Basic/DiagnosticKinds.def lib/AST/Decl.cpp lib/Sema/Sema.h lib/Sema/SemaDecl.cpp lib/Sema/SemaDeclObjC.cpp lib/Sema/SemaExpr.cpp lib/Sema/SemaExprObjC.cpp test/Sema/alias-test-1.m

[Steve Naroff]

On Apr 1, 2008, at 5:24 PM, Chris Lattner wrote:

> On Apr 1, 2008, at 4:04 PM, Steve Naroff wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=49058&view=rev
>> Log:
>> Fairly large "cleaup" related to changing ObjCCompatibleAliasDecl  
>> superclass (to inherit from NamedDecl, instead of ScopedDecl).
>>
>> - Added a DenseMap to associate an IdentifierInfo with the  
>> ObjCCompatibleAliasDecl.
>> - Renamed LookupScopedDecl->LookupDecl and changed it's return type  
>> to Decl. Also added lookup for ObjCCompatibleAliasDecl's.
>> - Removed Sema::LookupInterfaceDecl(). Converted clients to used  
>> LookupDecl().
>> - Some minor indentation changes.
>>
>> Will deal with ObjCInterfaceDecl and getObjCInterfaceDecl() in a  
>> separate commit...
>
> Nice!  Some trivial nits:
>
>>
>> +  /// ObjCAliasDecls - Keep track of all class declarations declared
>> +  /// with @compatibility_alias, so that we can emit errors on  
>> duplicates and
>> +  /// find the declarations when needed. This construct is ancient  
>> and will
>> +  /// likely never be seen. Nevertheless, it is here for  
>> compatibility.
>> +  typedef llvm::DenseMap<IdentifierInfo*,  
>> ObjCCompatibleAliasDecl*> ObjCAliasTy;
>
> If you make the key be "const IdentifierInfo*", then you can change:
>

Done...

>> Sema::DeclTy *Sema::isTypeName(const IdentifierInfo &II, Scope *S)  
>> const {
>>  Decl *IIDecl = II.getFETokenInfo<Decl>();
>>  // Find first occurance of none-tagged declaration
>> -  while(IIDecl && IIDecl->getIdentifierNamespace() !=  
>> Decl::IDNS_Ordinary)
>> +  while (IIDecl && IIDecl->getIdentifierNamespace() !=  
>> Decl::IDNS_Ordinary)
>>    IIDecl = cast<ScopedDecl>(IIDecl)->getNext();
>> -  if (!IIDecl)
>> +
>> +  if (!IIDecl) {
>> +    if (getLangOptions().ObjC1) {
>> +      // @interface and @compatibility_alias result in new type  
>> references.
>> +      // Creating a class alias is *extremely* rare.
>> +      ObjCAliasTy::const_iterator I =  
>> ObjCAliasDecls.find((IdentifierInfo*)&II);
>
> ... this to avoid a cast.
>

Done...

> Also, can 'isTypeName' just call LookupDecl?
>

I considered this, however LookupDecl calls LazilyCreateBuiltin (which  
I didn't think made sense when being called from isTypeName).

If I am wrong (and this is harmless), then I can make the change.
If not, we could consider complicating the API a bit.

Yet another solution I considered was to have LookupDecl/isTypeName  
call a helper to walk the scope change (this would give us the single  
point of control we'd like).

What's your preference?

>> +/// getObjCInterfaceDecl - Look up a for a class declaration in  
>> the scope.
>> +/// return 0 if one not found.
>> +/// FIXME: removed this when ObjCInterfaceDecl's aren't  
>> ScopedDecl's.
>> +ObjCInterfaceDecl *Sema::getObjCInterfaceDecl(IdentifierInfo *Id) {
>>  ScopedDecl *IDecl;
>>  // Scan up the scope chain looking for a decl that matches this  
>> identifier
>>  // that is in the appropriate namespace.
>> +  for (IDecl = Id->getFETokenInfo<ScopedDecl>(); IDecl;
>>       IDecl = IDecl->getNext())
>>    if (IDecl->getIdentifierNamespace() == Decl::IDNS_Ordinary)
>>      break;
>
> It would be nice to change this to call LookupDecl as well, just to  
> have a single place that walks the scope chains.
>

Agreed. I plan on tweaking this later...

snaroff

> Overall, great cleanup!
>
> -Chris