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

[Usman Akinyemi via Phabricator via llvm-commits]

Unique_Usman added a comment.

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.



================
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.
----------------
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. 



================
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:
> 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. 


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

https://reviews.llvm.org/D146759