[cfe-commits] r164861 - in /cfe/trunk: bindings/xml/comment-xml-schema.rng tools/libclang/CXComment.cpp

[Fariborz Jahanian]

On Sep 29, 2012, at 1:26 AM, Dmitri Gribenko wrote:

> On Sat, Sep 29, 2012 at 1:35 AM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> Author: fjahanian
>> Date: Fri Sep 28 17:35:49 2012
>> New Revision: 164861
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=164861&view=rev
>> Log:
>> [Doc parsing] Add availability information to generated Comment XML.
>> (I still need to add a test once I figure it out).
>> Reviewed off-line by Doug. // rdar://12378879
> 
> Hi Fariborz,
> 
> Nice to see you working on this!

Nice to see you reviewing it!

> 
> Please take a look at the attached patch that fixes some code style
> issues and apply it as you see it fit.
> 
Sure.

> ==================
>> --- cfe/trunk/bindings/xml/comment-xml-schema.rng (original)
>> +++ cfe/trunk/bindings/xml/comment-xml-schema.rng Fri Sep 28 17:35:49 2012
>> @@ -79,6 +79,9 @@
>>       <optional>
>>         <ref name="Parameters" />
>>       </optional>
>> +      <zeroOrMore>
>> +        <ref name="Attribute" />
>> +      </zeroOrMore>
>>       <optional>
>>         <ref name="ResultDiscussion" />
>>       </optional>
>> @@ -284,6 +287,39 @@
>>     </element>
>>   </define>
>> 
>> +  <define name="Attribute">
>> +   <element name="Availability">
>> +      <attribute name="distribution">
>> +          <data type="string" />
>> +      </attribute>
>> +      <optional>
>> +        <element name="IntroducedInVersion">
>> +          <data type="float" />
>> +        </element>
>> +      </optional>
>> +      <optional>
>> +        <element name="DeprecatedInVersion">
>> +          <data type="float" />
>> +        </element>
>> +      </optional>
>> +      <optional>
>> +        <element name="RemovedAfterVersion">
>> +          <data type="float" />
>> +        </element>
>> +      </optional>
>> +      <optional>
>> +        <element name="DeprecationSummary">
>> +          <data type="string" />
>> +        </element>
>> +      </optional>
>> +      <optional>
>> +        <element name="Unavailable">
>> +          <data type="boolean" />
>> +        </element>
>> +      </optional>
>> +   </element>
>> +  </define>
>> +
>>   <define name="Abstract">
>>     <element name="Abstract">
>>       <zeroOrMore>
> 
> Please add tests for schema changes to test/Index/comment-xml-schema.c.

Sure. I wan't aware of it.

> 
>> Modified: cfe/trunk/tools/libclang/CXComment.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CXComment.cpp?rev=164861&r1=164860&r2=164861&view=diff
>> ==============================================================================
>> --- cfe/trunk/tools/libclang/CXComment.cpp (original)
>> +++ cfe/trunk/tools/libclang/CXComment.cpp Fri Sep 28 17:35:49 2012
>> @@ -1170,7 +1170,62 @@
>>     visit(Parts.Returns);
>>     Result << "</ResultDiscussion>";
>>   }
>> -
>> +
>> +  if (DI->ThisDecl->hasAttrs()) {
>> +    const AttrVec &Attrs = DI->ThisDecl->getAttrs();
>> +    for (unsigned i = 0, e = Attrs.size(); i != e;) {
>> +      const AvailabilityAttr *AA = dyn_cast<AvailabilityAttr>(Attrs[i++]);
>> +      if (!AA)
>> +        continue;
>> +      // availability attribute info.
>> +
>> +      Result << "<Availability";
>> +      StringRef distribution;
>> +      if (AA->getPlatform()) {
>> +        distribution = AA->getPlatform()->getName();
>> +        if (distribution == "macosx")
>> +          distribution = "OSX";
>> +        else
>> +          distribution = "iOS";
>> +      }
> 
> Do we need to do this translation here?  Why do we map everything that
> is not "macosx" to "iOS"?

Yes, translation is necessary. There are currently macosx and ios as allowed platforms in availability macros. I will probably check for "ios" and assert for anything else.


> 
>> +
>> +      Result << " distribution=\"";
>> +      Result << distribution;
>> +      Result << "\">";
>> +      VersionTuple IntroducedInVersion = AA->getIntroduced();
>> +      if (!IntroducedInVersion.empty()) {
>> +        Result << " <IntroducedInVersion>";
> 
> These spaces before the tag are not needed, I removed them in the
> attached patch.
> 
>> +        Result << IntroducedInVersion.getAsString();
>> +        Result << "</IntroducedInVersion>";
>> +      }
>> +      VersionTuple DeprecatedInVersion = AA->getDeprecated();
>> +      if (!DeprecatedInVersion.empty()) {
>> +        Result << " <DeprecatedInVersion>";
>> +        Result << DeprecatedInVersion.getAsString();
>> +        Result << "</DeprecatedInVersion>";
>> +      }
>> +      VersionTuple RemovedAfterVersion = AA->getObsoleted();
>> +      if (!RemovedAfterVersion.empty()) {
>> +        Result << " <RemovedAfterVersion>";
>> +        Result << RemovedAfterVersion.getAsString();
>> +        Result << "</RemovedAfterVersion>";
>> +      }
>> +      StringRef DeprecationSummary = AA->getMessage();
>> +      if (!DeprecationSummary.empty()) {
>> +        Result << " <DeprecationSummary>";
>> +        Result << DeprecationSummary;
>> +        Result << "</DeprecationSummary>";
>> +      }
>> +      Result << " <Unavailable>";
>> +      if (AA->getUnavailable())
>> +        Result << "true";
>> +      else
>> +        Result << "false";
>> +      Result << "</Unavailable>";
>> +      Result << " </Availability>";
>> +    }
>> +  }
>> +
>>   {
>>     bool StartTagEmitted = false;
>>     for (unsigned i = 0, e = Parts.MiscBlocks.size(); i != e; ++i) {
> 
> Please add tests for CXComment.cpp changes to
> test/Index/annotate-comments.cpp.  It is a bit crowded and slow to
> FileCheck already, so you might want to put tests for availability
> attrs into new file, something like
> annotate-comments-availability-attrs.cpp.

I was planning to add a test. I got bogged down trying to add a workable test to annotate-comments.cpp.
I will add one soon though.

- Thanks, Fariborz

> 
> Dmitri
> 
> -- 
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
> <cxcomment-code-style-v1.patch>