[cfe-commits] r147408 - in /cfe/trunk: include/clang/AST/DeclObjC.h lib/AST/ASTImporter.cpp lib/AST/DeclObjC.cpp lib/AST/DumpXML.cpp lib/Rewrite/RewriteObjC.cpp lib/Sema/SemaCodeComplete.cpp lib/Sema/SemaDeclObjC.cpp lib/Serialization/ASTReaderDecl.cpp lib/Serialization/ASTWriterDecl.cpp tools/libclang/CIndex.cpp tools/libclang/IndexDecl.cpp tools/libclang/IndexingContext.cpp

[Douglas Gregor]

On Jan 1, 2012, at 1:33 PM, Fariborz Jahanian wrote:

> 
> On Jan 1, 2012, at 11:29 AM, Douglas Gregor wrote:
> 
>> Author: dgregor
>> Date: Sun Jan  1 13:29:29 2012
>> New Revision: 147408
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=147408&view=rev
>> Log:
>> Move the data that corresponds to the definition of a protocol into a
>> separately-allocated DefinitionData structure. Introduce various
>> functions that will help with the separation of declarations from
>> definitions (isThisDeclarationADefinition(), hasDefinition(),
>> getDefinition()).
>> [snip]
>> Modified: cfe/trunk/lib/AST/ASTImporter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=147408&r1=147407&r2=147408&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/ASTImporter.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTImporter.cpp Sun Jan  1 13:29:29 2012
>> @@ -3119,7 +3119,7 @@
>>  }
>> 
>>  ObjCProtocolDecl *ToProto = MergeWithProtocol;
>> -  if (!ToProto || ToProto->isForwardDecl()) {
>> +  if (!ToProto || !ToProto->hasDefinition()) {
>>    if (!ToProto) {
>>      ToProto = ObjCProtocolDecl::Create(Importer.getToContext(), DC,
>>                                         Name.getAsIdentifierInfo(), Loc,
>> @@ -3127,9 +3127,12 @@
>>                                         D->isInitiallyForwardDecl());
>>      ToProto->setLexicalDeclContext(LexicalDC);
>>      LexicalDC->addDeclInternal(ToProto);
>> -      if (D->isInitiallyForwardDecl() && !D->isForwardDecl())
>> +      if (D->isInitiallyForwardDecl() && D->hasDefinition())
> 
> Why the new condition is semantically equivalent to the old. If protocol is forward decl. with a definition somewhere,
> old condition returns 'false' while the new condition return 'true'.  I guess I don't understand from the context of use
> why one can be unconditionally  replaced with the negation of the other everywhere.

isForwardDecl() is a bit confusing. I had thought it would mean that this particular declaration is a forward declaration, and that a definition could be elsewhere. However, isForwardDecl() actually meant "there is no definition anywhere in the translation unit", since there was only ever one ObjCProtocolDecl for a given protocol, and as soon as we saw a definition we'd flip the bit to make isForwardDecl() return false.

That's why I removed isForwardDecl(): it's name and semantics didn't match up, and I audited all of its callers to make sure they use the appropriate predicate. One can now check isThisDeclarationADefinition() or hasDefinition() to check whether this declaration or any declaration is the definition, respectively. (I did the same thing with ObjCInterfaceDecl)

> 
>>        ToProto->completedForwardDecl();
>>    }
>> +    if (!ToProto->hasDefinition())
>> +      ToProto->startDefinition();
>> +    
>>    Importer.Imported(D, ToProto);
>> 
>>    // Import protocols
>> 
>> Modified: cfe/trunk/lib/AST/DeclObjC.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=147408&r1=147407&r2=147408&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/DeclObjC.cpp (original)
>> +++ cfe/trunk/lib/AST/DeclObjC.cpp Sun Jan  1 13:29:29 2012
>> @@ -1003,9 +1003,19 @@
>>  return NULL;
>> }
>> 
>> +void ObjCProtocolDecl::allocateDefinitionData() {
>> +  assert(!Data && "Protocol already has a definition!");
>> +  Data = new (getASTContext()) DefinitionData;  
>> +}
>> +
>> +void ObjCProtocolDecl::startDefinition() {
>> +  allocateDefinitionData();
>> +}
>> +
>> void ObjCProtocolDecl::completedForwardDecl() {
>> -  assert(isForwardDecl() && "Only valid to call for forward refs");
>> +  assert(!hasDefinition() && "Only valid to call for forward refs");
>>  isForwardProtoDecl = false;
>> +  startDefinition();
>>  if (ASTMutationListener *L = getASTContext().getASTMutationListener())
>>    L->CompletedObjCForwardRef(this);
>> }
>> 
>> Modified: cfe/trunk/lib/AST/DumpXML.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DumpXML.cpp?rev=147408&r1=147407&r2=147408&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/DumpXML.cpp (original)
>> +++ cfe/trunk/lib/AST/DumpXML.cpp Sun Jan  1 13:29:29 2012
>> @@ -819,7 +819,6 @@
>> 
>>  // ObjCProtocolDecl
>>  void visitObjCProtocolDeclAttrs(ObjCProtocolDecl *D) {
>> -    setFlag("forward_decl", D->isForwardDecl());
>>  }
> 
> Is this method still needed?

Nope, good catch. Fixed in r147416.

	- Doug