>>>>> Do we need a new subclass? I'm not actually sure what this needs
>>> that a ValueDecl doesn't provide, so we could possibly just use a
>>> ValueDecl.
>>
>> If that works, please go for it. Please add a big comment above te
>> ValueDecl class indicating that ObjC uses it this way though.
>
> ValueDecl's constructor is currently private, so I've added a public
> Create() method. This seems to work. I wasn't using anything
> specific to the param decl, so (after fixing a load of compiler
> complaints) it is working again.
Ok, this looks good. You should probably use a specific Decl enum
value for them though. Would it be useful to add a subclass of
ValueDecl just to support easy use of :
if (isa<FooDecl>(X))
That would check for the enum? In fact, I really do think this would
be preferable, just because the name ValueDecl really does seem like
an abstract type. What do you think?
>> Take a look at how name lookup is currently working and how builtins
>> are lazily created, you should be able to do something similar.
>
> This is fine for codegen. Will other AST consumers object if
> implicit parameter decls aren't there if they are not referenced?
Builtins are correct in the ast. They are lazily generated by sema,
but sema updates the AST and namelookup appropriately.
>>> ry rare to have a class which does not contain at least one
>>> reference to self. Most classes will override -init, and the common
>>> pattern for this is to start the method with:
>>>
>>> if (nil == (self = [self init])) return nil;
>>
>> Sure, but we'd need to make one decl for self in *every method* that
>> references it, not once per class.
>
> Can we not have one decl object instance per class, but one decl map
> entry (pointing to this instance)?
As we discussed on irc, this is a totally awesome idea. Lets talk
about this as a followup patch though.
>>> [1] These should probably have a common superclass, since they are
>>> almost identical - in fact an ObjCImplementationDecl could inherit
>>> from ObjCCategoryImplDecl, since a class is effectively a category
>>> which is allowed to declare instance variables.
>>
>> I think it would be great to unify some of this stuff. There is a
>> lot
>> of awkward code in the objc front-end for groking this stuff that
>> should be unified, but I haven't figured out how. Suggestions
>> welcome :)
>
> I still have about 2000 lines of uncommitted diffs, but when my tree
> has sync'd up with trunk a bit better then I'll take a look at the
> front end a bit and see what can be unified. There are a few other
> strange things there, such as the 'isArrow' property on ivar refs
> which, unless I am missing something important, can never be false.
Ok! We welcome cleanup for the ObjC ASTs!
Some other thoughts about your patch:
+++ include/clang/AST/DeclObjC.h (working copy)
@@ -91,7 +91,9 @@
// The following are only used for method definitions, null
otherwise.
// FIXME: space savings opportunity, consider a sub-class.
Stmt *Body;
- ParmVarDecl *SelfDecl;
+ // Decls for implicit parameters
+ ValueDecl *ImplicitParamInfo[4];
+ unsigned NumImplicitParams;
Please turn this into SelfDecl/CmdDecl/etc with explicit getters/
setters instead of providing iteration with a count.
+++ lib/CodeGen/CodeGenFunction.cpp (working copy)
@@ -27,7 +27,11 @@
CodeGenFunction::CodeGenFunction(CodeGenModule &cgm)
: CGM(cgm), Target(CGM.getContext().Target), SwitchInsn(NULL),
- CaseRangeBlock(NULL) {}
+ CaseRangeBlock(NULL) {
+ LLVMIntTy = ConvertType(getContext().IntTy);
+ LLVMPointerWidth = static_cast<unsigned>(
+
getContext
().getTypeSize(getContext().getPointerType(getContext().VoidTy)));
+}
Watch out for 80 columns. You can directly get the size of a pointer
from TargetData instead of cons'ing up a pointer type and asking for
its size.
+const llvm::Type *CodeGenTypes::ConvertReturnType(QualType T) {
+ if (T->isVoidType())
+ return llvm::Type::VoidTy; // Result of function uses llvm
void.
+ else
+ return ConvertType(T);
+}
Please indent by 2.
Otherwise the patch is looking great. Thanks for tolerating me ;)
-Chris