[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 requested changes to this revision.
zixuw added a comment.
This revision now requires changes to proceed.

Thanks for creating the review!

Please keep the commit title and message concise and describe the change. See https://llvm.org/docs/DeveloperPolicy.html#commit-messages



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


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


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


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

https://reviews.llvm.org/D146759