[cfe-commits] r60634 - in /cfe/trunk: include/clang/AST/DeclObjC.h lib/AST/DeclObjC.cpp lib/Sema/Sema.h lib/Sema/SemaDeclObjC.cpp test/SemaObjC/property-category-3.m
steve naroff
snaroff at apple.com
Wed Jan 7 06:34:57 PST 2009
On Jan 7, 2009, at 1:47 AM, Daniel Dunbar wrote:
> Fariborz (& Steve),
>
> I know this is how some other existing code was being handled, but I
> am a little concerned about this implementation strategy (copying the
> properties into the class). This feels counter to clang's general
> strategy of having the AST closely reflect the source. In addition, it
> seems like this means more malloc trashing than is really necessary.
>
> My question is, how much work would it be to *not* copy the properties
> in this fashion? Is it more complicated than just rewriting the type
> checking and the property lookup mechanisms?
>
It's hard for me to estimate how much work, since there are other
related issues (see <rdar://problem/5957689> clang ObjC AST: @property
should not inject method decls into the enclosing interface).
I totally agree that we should avoid the copying and change the lookup
methods. That said, it took quite some time to shake out all the
"method not found" warnings (which can lead to bad code gen). Given
ObjC's loosely typed heritage, the lookup rules are sometimes odd
(based on whatever GCC does).
So...I've been avoiding fixing <rdar://problem/5957689>, since it is
largely "cleanup" (i.e. it doesn't have any end-user benefit).
It would be nice to avoid creating more/related problems (as you
suggest).
> I can imagine that this will slow down the property lookup, but I
> think the common usage scenario is that many classes are declared but
> few will actually participate in lookup. And there is always the
> option of lazily computing the lookup information if we really need
> the best of both worlds.
>
I'm not concerned about the speed of property lookup. With or without
the copying, I'm sure property lookup can be made efficient
(especially since the cost should be proportional to the number of
properties used, vs. declared).
I imagine the copying was originally intended to simplify the lookup
mechanism (not speed it up). Fariborz, is this correct? What's your
perspective?
snaroff
> - Daniel
>
> On Sat, Dec 6, 2008 at 3:03 PM, Fariborz Jahanian
> <fjahanian at apple.com> wrote:
>> Author: fjahanian
>> Date: Sat Dec 6 17:03:39 2008
>> New Revision: 60634
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=60634&view=rev
>> Log:
>> Use of properties declared in protocols in the category
>> via the category's protocol list1s, with appropriate
>> diagnsostics and a test case.
>>
>> Added:
>> cfe/trunk/test/SemaObjC/property-category-3.m
>> Modified:
>> cfe/trunk/include/clang/AST/DeclObjC.h
>> cfe/trunk/lib/AST/DeclObjC.cpp
>> cfe/trunk/lib/Sema/Sema.h
>> cfe/trunk/lib/Sema/SemaDeclObjC.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=60634&r1=60633&r2=60634&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclObjC.h Sat Dec 6 17:03:39 2008
>> @@ -883,6 +883,8 @@
>>
>> void addProperties(ObjCPropertyDecl **Properties, unsigned
>> NumProperties);
>>
>> + void mergeProperties(ObjCPropertyDecl **Properties, unsigned
>> NumProperties);
>> +
>> void addPropertyMethods(ASTContext &Context,
>> ObjCPropertyDecl* Property,
>> llvm::SmallVector<ObjCMethodDecl*, 32>
>> &insMethods,
>>
>> Modified: cfe/trunk/lib/AST/DeclObjC.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=60634&r1=60633&r2=60634&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/lib/AST/DeclObjC.cpp (original)
>> +++ cfe/trunk/lib/AST/DeclObjC.cpp Sat Dec 6 17:03:39 2008
>> @@ -513,6 +513,31 @@
>> ::addPropertyMethods(this, Context, property, insMethods, InsMap);
>> }
>>
>> +/// mergeProperties - Adds properties to the end of list of
>> current properties
>> +/// for this category.
>> +
>> +void ObjCCategoryDecl::mergeProperties(ObjCPropertyDecl
>> **Properties,
>> + unsigned NumNewProperties) {
>> + if (NumNewProperties == 0) return;
>> +
>> + if (PropertyDecl) {
>> + ObjCPropertyDecl **newPropertyDecl =
>> + new ObjCPropertyDecl*[NumNewProperties + NumPropertyDecl];
>> + ObjCPropertyDecl **buf = newPropertyDecl;
>> + // put back original properties in buffer.
>> + memcpy(buf, PropertyDecl,
>> NumPropertyDecl*sizeof(ObjCPropertyDecl*));
>> + // Add new properties to this buffer.
>> + memcpy(buf+NumPropertyDecl, Properties,
>> + NumNewProperties*sizeof(ObjCPropertyDecl*));
>> + delete[] PropertyDecl;
>> + PropertyDecl = newPropertyDecl;
>> + NumPropertyDecl += NumNewProperties;
>> + }
>> + else {
>> + addProperties(Properties, NumNewProperties);
>> + }
>> +}
>> +
>> /// addPropertyMethods - Goes through list of properties declared
>> in this class
>> /// and builds setter/getter method declartions depending on the
>> setter/getter
>> /// attributes of the property.
>>
>> Modified: cfe/trunk/lib/Sema/Sema.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.h?rev=60634&r1=60633&r2=60634&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/lib/Sema/Sema.h (original)
>> +++ cfe/trunk/lib/Sema/Sema.h Sat Dec 6 17:03:39 2008
>> @@ -1075,10 +1075,10 @@
>> const IdentifierInfo *Name);
>> void ComparePropertiesInBaseAndSuper(ObjCInterfaceDecl *IDecl);
>>
>> - void MergeProtocolPropertiesIntoClass(ObjCInterfaceDecl *IDecl,
>> + void MergeProtocolPropertiesIntoClass(Decl *CDecl,
>> DeclTy *MergeProtocols);
>>
>> - void MergeOneProtocolPropertiesIntoClass(ObjCInterfaceDecl *IDecl,
>> + void MergeOneProtocolPropertiesIntoClass(Decl *CDecl,
>> ObjCProtocolDecl *PDecl);
>>
>> virtual void ActOnAtEnd(SourceLocation AtEndLoc, DeclTy *classDecl,
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=60634&r1=60633&r2=60634&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Sat Dec 6 17:03:39 2008
>> @@ -311,11 +311,35 @@
>>
>> /// MergeOneProtocolPropertiesIntoClass - This routine goes thru
>> the list
>> /// of properties declared in a protocol and adds them to the list
>> -/// of properties for current class if it is not there already.
>> +/// of properties for current class/category if it is not there
>> already.
>> void
>> -Sema::MergeOneProtocolPropertiesIntoClass(ObjCInterfaceDecl *IDecl,
>> +Sema::MergeOneProtocolPropertiesIntoClass(Decl *CDecl,
>> ObjCProtocolDecl *PDecl) {
>> llvm::SmallVector<ObjCPropertyDecl*, 16> mergeProperties;
>> + ObjCInterfaceDecl *IDecl =
>> dyn_cast_or_null<ObjCInterfaceDecl>(CDecl);
>> + if (!IDecl) {
>> + // Category
>> + ObjCCategoryDecl *CatDecl =
>> static_cast<ObjCCategoryDecl*>(CDecl);
>> + assert (CatDecl && "MergeOneProtocolPropertiesIntoClass");
>> + for (ObjCProtocolDecl::classprop_iterator P = PDecl-
>> >classprop_begin(),
>> + E = PDecl->classprop_end(); P != E; ++P) {
>> + ObjCPropertyDecl *Pr = (*P);
>> + ObjCCategoryDecl::classprop_iterator CP, CE;
>> + // Is this property already in category's list of properties?
>> + for (CP = CatDecl->classprop_begin(), CE = CatDecl-
>> >classprop_end();
>> + CP != CE; ++CP)
>> + if ((*CP)->getIdentifier() == Pr->getIdentifier())
>> + break;
>> + if (CP == CE)
>> + // Add this property to list of properties for thie class.
>> + mergeProperties.push_back(Pr);
>> + else
>> + // Property protocol already exist in class. Diagnose any
>> mismatch.
>> + DiagnosePropertyMismatch((*CP), Pr, PDecl->getIdentifier());
>> + }
>> + CatDecl->mergeProperties(&mergeProperties[0],
>> mergeProperties.size());
>> + return;
>> + }
>> for (ObjCProtocolDecl::classprop_iterator P = PDecl-
>> >classprop_begin(),
>> E = PDecl->classprop_end(); P != E; ++P) {
>> ObjCPropertyDecl *Pr = (*P);
>> @@ -337,13 +361,39 @@
>>
>> /// MergeProtocolPropertiesIntoClass - This routine merges properties
>> /// declared in 'MergeItsProtocols' objects (which can be a class
>> or an
>> -/// inherited protocol into the list of properties for class 'IDecl'
>> +/// inherited protocol into the list of properties for class/
>> category 'CDecl'
>> ///
>>
>> void
>> -Sema::MergeProtocolPropertiesIntoClass(ObjCInterfaceDecl *IDecl,
>> +Sema::MergeProtocolPropertiesIntoClass(Decl *CDecl,
>> DeclTy *MergeItsProtocols) {
>> Decl *ClassDecl = static_cast<Decl *>(MergeItsProtocols);
>> + ObjCInterfaceDecl *IDecl =
>> dyn_cast_or_null<ObjCInterfaceDecl>(CDecl);
>> +
>> + if (!IDecl) {
>> + // Category
>> + ObjCCategoryDecl *CatDecl =
>> static_cast<ObjCCategoryDecl*>(CDecl);
>> + assert (CatDecl && "MergeProtocolPropertiesIntoClass");
>> + if (ObjCCategoryDecl *MDecl =
>> dyn_cast<ObjCCategoryDecl>(ClassDecl)) {
>> + for (ObjCCategoryDecl::protocol_iterator P = MDecl-
>> >protocol_begin(),
>> + E = MDecl->protocol_end(); P != E; ++P)
>> + // Merge properties of category (*P) into IDECL's
>> + MergeOneProtocolPropertiesIntoClass(CatDecl, *P);
>> +
>> + // Go thru the list of protocols for this category and
>> recursively merge
>> + // their properties into this class as well.
>> + for (ObjCCategoryDecl::protocol_iterator P = CatDecl-
>> >protocol_begin(),
>> + E = CatDecl->protocol_end(); P != E; ++P)
>> + MergeProtocolPropertiesIntoClass(CatDecl, *P);
>> + } else {
>> + ObjCProtocolDecl *MD = cast<ObjCProtocolDecl>(ClassDecl);
>> + for (ObjCProtocolDecl::protocol_iterator P = MD-
>> >protocol_begin(),
>> + E = MD->protocol_end(); P != E; ++P)
>> + MergeOneProtocolPropertiesIntoClass(CatDecl, (*P));
>> + }
>> + return;
>> + }
>> +
>> if (ObjCInterfaceDecl *MDecl =
>> dyn_cast<ObjCInterfaceDecl>(ClassDecl)) {
>> for (ObjCInterfaceDecl::protocol_iterator P = MDecl-
>> >protocol_begin(),
>> E = MDecl->protocol_end(); P != E; ++P)
>> @@ -1063,8 +1113,8 @@
>> // By the same token, they are also used to add new properties. No
>> // need to compare the added property to those in the class.
>>
>> - // FIXME: If we merge properties into class we should probably
>> - // merge them into category as well?
>> + // Merge protocol properties into category
>> + MergeProtocolPropertiesIntoClass(C, C);
>> for (ObjCCategoryDecl::classprop_iterator i = C-
>> >classprop_begin(),
>> e = C->classprop_end(); i != e; ++i) {
>> diagnosePropertySetterGetterMismatch((*i), InsMap[(*i)-
>> >getGetterName()],
>>
>> Added: cfe/trunk/test/SemaObjC/property-category-3.m
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/property-category-3.m?rev=60634&view=auto
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/test/SemaObjC/property-category-3.m (added)
>> +++ cfe/trunk/test/SemaObjC/property-category-3.m Sat Dec 6
>> 17:03:39 2008
>> @@ -0,0 +1,21 @@
>> + at protocol P
>> + @property(readonly) int X;
>> + at end
>> +
>> + at protocol P1<P>
>> + @property (copy) id ID;
>> + at end
>> +
>> + at interface I
>> + at end
>> +
>> + at interface I (Cat) <P>
>> + at property float X; // expected-warning {{property type 'float'
>> does not match property type inherited from 'P'}}
>> + at end
>> +
>> + at interface I (Cat2) <P1>
>> + at property (retain) id ID; // expected-warning {{property 'ID'
>> 'copy' attribute does not match the property inherited from 'P1'}}
>> + at end
>> +
>> +
>> +
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list