[cfe-commits] r42311 - in /cfe/trunk: AST/Decl.cpp Parse/MinimalAction.cpp Parse/ParseObjc.cpp Sema/Sema.h Sema/SemaDecl.cpp clang.xcodeproj/project.pbxproj include/clang/AST/ASTContext.h include/clang/AST/Decl.h include/clang/Basic/DiagnosticKinds.def include/clang/Parse/Action.h test/Sema/class-def-test-1.m test/Sema/class-impl-1.m

[Chris Lattner]

On Sep 25, 2007, at 11:38 AM, Fariborz Jahanian wrote:
> Author: fjahanian
> Date: Tue Sep 25 13:38:09 2007
> New Revision: 42311
>
> URL: http://llvm.org/viewvc/llvm-project?rev=42311&view=rev
> Log:
> This patch introduces a new class to keep track of class  
> implementation info. It also adds more
> semantic checks for class and protocol declarations. Test cases are  
> good indications of kind of
> checking being done in this patch.

Very nice Fariborz,

Here's a question for you and Steve:

> ====================================================================== 
> ========
> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> +++ cfe/trunk/include/clang/AST/ASTContext.h Tue Sep 25 13:38:09 2007
> @@ -39,6 +39,7 @@
>    llvm::DenseMap<const RecordDecl*, const RecordLayout*>  
> RecordLayoutInfo;
>    llvm::DenseMap<const IdentifierInfo*, ObjcInterfaceDecl*>  
> ClassNameInfo;
>    llvm::DenseMap<const IdentifierInfo*, ObjcProtocolDecl*>  
> ProtocolNameInfo;
> +  llvm::SmallVector<ObjcImplementationDecl*, 8>  
> ImplementationClassInfo;
>    RecordDecl *CFConstantStringTypeDecl;
>  public:
>
> @@ -172,6 +173,16 @@
>                              ObjcProtocolDecl* ProtocolDecl)
>    { ProtocolNameInfo[ProtocolName] = ProtocolDecl; }
>
> +  ObjcImplementationDecl* getObjcImplementationClass(unsigned ix) {
> +    return ImplementationClassInfo[ix];
> +  }
> +  void setObjcImplementationClass(ObjcImplementationDecl* ImplDecl) {
> +    ImplementationClassInfo.push_back(ImplDecl);
> +  }
> +  unsigned sizeObjcImplementationClass() const {
> +    return ImplementationClassInfo.size();
> +  }
> +

This is a bit strange to me.  It has no analog in C.  My  
understanding is that this:

> +Sema::DeclTy *Sema::ObjcStartClassImplementation(Scope *S,
> +                      SourceLocation AtClassImplLoc,
> +                      IdentifierInfo *ClassName, SourceLocation  
> ClassLoc,
> +                      IdentifierInfo *SuperClassname,
> +                      SourceLocation SuperClassLoc) {
> +  ObjcInterfaceDecl* IDecl = 0;
> +  // Check for another declaration kind with the same name.
> +  ScopedDecl *PrevDecl = LookupScopedDecl(ClassName,  
> Decl::IDNS_Ordinary,
> +                                          ClassLoc, S);

Looks up the forward declaration for the implementation, which is  
checked and then:

> +  if (PrevDecl && !isa<ObjcInterfaceDecl>(PrevDecl)) {
> +    Diag(ClassLoc, diag::err_redefinition_different_kind,
> +         ClassName->getName());
> +    Diag(PrevDecl->getLocation(), diag::err_previous_definition);
> +  }
> +  else {
> +    // Is there an interface declaration of this class; if not, warn!
> +    IDecl = Context.getObjCInterfaceDecl(ClassName);
> +    if (!IDecl)
> +      Diag(ClassLoc, diag::warn_undef_interface, ClassName->getName 
> ());
> +  }
> +
...
> +
> +  ObjcImplementationDecl* IMPDecl =
> +    new ObjcImplementationDecl(AtClassImplLoc, ClassName, SDecl);

an implementation decl object is made.

Would it be feasible to handle this like we do for struct forward  
definitions, where the definition of a struct goes back and fills in  
the declaration with the implementation info?  It seems strange to me  
that objc code like this:

@interface OBJECT @end
@interface INTF  : OBJECT
@end
@implementation INTF @end

Creates an 'ObjcInterfaceDecl' for INTF, then a separate  
'ObjcImplementationDecl' later, which isn't added to the scope info.   
If it were, you could drop this linear time lookup code:

> +  // Check that there is no duplicate implementation of this class.
> +  bool err = false;
> +  for (unsigned i = 0; i != Context.sizeObjcImplementationClass();  
> i++) {
> +    if (Context.getObjcImplementationClass(i)->getIdentifier() ==  
> ClassName) {
> +      Diag(ClassLoc, diag::err_dup_implementation_class, ClassName- 
> >getName());
> +      err = true;
> +      break;
> +    }
> +  }
> +  if (!err)
> +    Context.setObjcImplementationClass(IMPDecl);

and just turn this into a simple query, like we have for redefinition  
of structs.

Is this craziness?

-Chris