[PATCH] D146759: Declaration fragments for properties include additional property attributes that were not specified in the sources proposed solution

[Zixu Wang via Phabricator via llvm-commits]

zixuw added a comment.

In D146759#4217882 <https://reviews.llvm.org/D146759#4217882>, @Unique_Usman wrote:

> Also, if there is a manual/README.md file to better understand the DeclarationFragments.cpp/ExtractAPI, kindly point it out so, I can go through it, thank you.

The closest thing is the generated documentation: https://clang.llvm.org/doxygen/classclang_1_1extractapi_1_1DeclarationFragments.html and https://clang.llvm.org/doxygen/classclang_1_1extractapi_1_1DeclarationFragmentsBuilder.html.
Also the code and the inline comments should pretty much explain itself.



================
Comment at: clang/lib/ExtractAPI/DeclarationFragments.cpp:625
 
   const auto Attributes = Property->getPropertyAttributes();
   // Build the attributes if there is any associated with the property.
----------------
This is where the attributes for the property decl are retrieved.


================
Comment at: clang/lib/ExtractAPI/DeclarationFragments.cpp:628
+
+  if (Attributes == ObjCPropertyAttribute::kind_noattr) { //I changed the != to == as I guess we only want to generate the attribute only if it is the same as the one specified in the source 
     // No leading comma for the first attribute.
----------------
Unique_Usman wrote:
> Unique_Usman wrote:
> > zixuw wrote:
> > > zixuw wrote:
> > > > zixuw wrote:
> > > > > I don't think this is correct.
> > > > > `Attributes` is simply a bitfield indicator of which attributes are true for the property. `kind_noattr` are just zero bytes meaning there's no attribute at all.
> > > > > Effectively this change just skips rendering attributes for every property.
> > > > Please use inline comments to describe and explain the actual code logic. You can communicate your change and thought process here in the review thread. See https://llvm.org/docs/CodingStandards.html
> > > Please run the LLVM test suite after your change, and update existing tests/write new regression tests.
> > > See https://llvm.org/docs/TestingGuide.html
> > > 
> > > You would find out that this is wrong by running the ExtractAPI tests.
> > Thanks for the reply, my question now is, since you know more about the code than  anyone else, can you kindly point out the part of the code which get the property to be rendered and the function that rendered the property. 
> > 
> Thanks for pointing out the guide for testing. 
You are close! The surrounding lines in this method retrieves and builds the fragments for the attributes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146759/new/

https://reviews.llvm.org/D146759