This is an archive of the discontinued LLVM Phabricator instance.

Differential D146759

[ExtractAPI] Remove extra attributes in property declaration fragments
ClosedPublic

Authored by Unique_Usman on Mar 23 2023, 2:10 PM.

Details

Reviewers
zixuw
dang
samtebbs
ributzka
Commits
rG32b53cf9d0c8: [ExtractAPI] Remove extra attributes in property declaration fragments
Summary

Use getPropertyAttributesAsWritten instead of getPropertyAttributes
to get property attributes actually specified in the source code.
Resolves issue #61478.

https://reviews.llvm.org/D146759

Diff Detail

Event Timeline

Unique_Usman created this revision.Mar 23 2023, 2:10 PM
Herald added a reviewer: ributzka. · View Herald TranscriptMar 23 2023, 2:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
Unique_Usman requested review of this revision.Mar 23 2023, 2:10 PM
zixuw requested changes to this revision.Mar 23 2023, 2:30 PM

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

clang/lib/ExtractAPI/DeclarationFragments.cpp
644

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.

644

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

644

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.

This revision now requires changes to proceed.Mar 23 2023, 2:30 PM
Unique_Usman added a comment.Mar 23 2023, 2:56 PM

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.

clang/lib/ExtractAPI/DeclarationFragments.cpp
644

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.

644

Thanks for pointing out the guide for testing.

Unique_Usman added a comment.Mar 23 2023, 3:06 PM

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.

zixuw added a comment.Mar 23 2023, 3:34 PM
In 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.

clang/lib/ExtractAPI/DeclarationFragments.cpp
641

This is where the attributes for the property decl are retrieved.

644

You are close! The surrounding lines in this method retrieves and builds the fragments for the attributes.

Unique_Usman added a comment.Mar 23 2023, 3:39 PM

Thank you very much for this, I will go through the resources to get hold of what is going on, by the way, I am a first year student actually, I might not be good as you might have expected but trust me, I am always ready to learn.
Thank you I will revert back to you.

samtebbs added a comment.Mar 24 2023, 7:13 AM

Hi Usman, as zixuw said this doesn't look correct as-is. Are you basing this work on a github issue?

Unique_Usman added a comment.Mar 24 2023, 8:25 AM

Hi Samuel,

The issue was opened Daniel and among the good first issue on GitHub.

Am I able to answer your question about if I am basing this work on a GitHub issue?

Unique_Usman added a comment.Mar 25 2023, 5:58 AM

Hi @zixuw kindly check the question I asked in the github thread for the issue. Thank you. https://github.com/llvm/llvm-project/issues/61478

Unique_Usman updated this revision to Diff 508782.Mar 27 2023, 1:31 PM

changed the getPropertyAttributes() to getPropertyAttributesAsWritten() which is the function for getting the property declared by the user.

Unique_Usman added a comment.Mar 27 2023, 1:33 PM

Kindly Ignore the Contributing.md file, it is a changes that has been accepted but, not yet commited. Thank, I look forward to your respond.

zixuw requested changes to this revision.Mar 27 2023, 2:29 PM

Please update/add test cases.

CONTRIBUTING.md
10–18 ↗(On Diff #508782)

Do not include irrelevant changes.

clang/lib/ExtractAPI/DeclarationFragments.cpp
641–642

Do not use comments to describe the change you've made.

This revision now requires changes to proceed.Mar 27 2023, 2:29 PM
Unique_Usman added a comment.Mar 27 2023, 3:00 PM

Thanks for your reply sir. Regarding the comment, I would remove it sir.

Sir, about updating/adding the test cases, I have never written any test case before sir, kindly guide me through that sir?

zixuw added a comment.Mar 27 2023, 9:17 PM
In D146759#4225656, @Unique_Usman wrote:

Thanks for your reply sir. Regarding the comment, I would remove it sir.

Sir, about updating/adding the test cases, I have never written any test case before sir, kindly guide me through that sir?

I cannot hold your hands through everything. You can learn a lot by looking at other people's commits and previous commits. Go ahead and find some simple commits, and check out how other people are writing a patch with a test case, and how other people write the commit message.
ExtractAPI tests are located under clang/test/ExtractAPI/.

Harbormaster completed remote builds in B222103: Diff 508782.Mar 27 2023, 10:48 PM
Unique_Usman updated this revision to Diff 510361.Apr 2 2023, 11:41 AM
Unique_Usman retitled this revision from Declaration fragments for properties include additional property attributes that were not specified in the sources proposed solution to [clang][ExtractAPI] Handle Declaration fragments for properties to not include spurious additional attributes..
Unique_Usman edited the summary of this revision. (Show Details)

This change is mainly to ensure Declaration fragments for properties does not include additional attributes of other property that is not specified in the source.

Unique_Usman added a comment.Apr 2 2023, 11:49 AM

Hello @zixuw, kinly review it sir, I also updated the test as you made mentioned sir. Thank you sir.

Harbormaster completed remote builds in B223258: Diff 510361.Apr 2 2023, 12:15 PM
Unique_Usman updated this revision to Diff 510366.Apr 2 2023, 2:02 PM
Harbormaster completed remote builds in B223264: Diff 510366.Apr 2 2023, 3:04 PM
zixuw added a comment.Apr 3 2023, 1:08 PM

Hi Usman! This is looking very good and complete now! Thanks for taking the time to learn and fix this.

nit: it's a good practice to keep the commit message title short (<70 chars) and explain a bit more in the message body. Also good to mention the issue if there is one. For example:

[ExtractAPI] Remove extra attributes in property declaration fragments

Use `getPropertyAttributesAsWritten` instead of `getPropertyAttributes`
to get property attributes actually specified in the source code.
Resolves issue #61478.

Differential Revision: https://reviews.llvm.org/D146759
Unique_Usman updated this revision to Diff 510663.Apr 3 2023, 7:20 PM
Unique_Usman retitled this revision from [clang][ExtractAPI] Handle Declaration fragments for properties to not include spurious additional attributes. to [ExtractAPI] Remove extra attributes in property declaration fragments.
Unique_Usman edited the summary of this revision. (Show Details)
Unique_Usman added a comment.Apr 3 2023, 7:23 PM

If this is okay, kindly help to make the commit sir. My email is usmanakinyemi202@gmail.com

Harbormaster completed remote builds in B223478: Diff 510663.Apr 3 2023, 8:10 PM
dang added a comment.Apr 4 2023, 7:55 AM

Do we still have a test that ensures that actually writing the property attributes manually results in them being added to the declaration fragments?

Unique_Usman added a comment.Apr 4 2023, 8:22 AM
In D146759#4243366, @dang wrote:

Do we still have a test that ensures that actually writing the property

In D146759#4243366, @dang wrote:

Do we still have a test that ensures that actually writing the property attributes manually results in them being added to the declaration fragments?

Yes, I updated the prevous test by using the content of the input files in each of the test. The declaration fragments of each of them is generated/added without additional attributes that is not specified in the source.

dang accepted this revision.Apr 4 2023, 9:51 AM

LGTM

Unique_Usman added a comment.Apr 4 2023, 9:55 AM
In D146759#4243717, @dang wrote:

LGTM

Kindly help to make the commit sir. My email is usmanakinyemi202@gmail.com

I hope to jump on another ExtractAPI issues. Thanks.

zixuw accepted this revision.Apr 4 2023, 9:56 AM
This revision is now accepted and ready to land.Apr 4 2023, 9:56 AM
Closed by commit rG32b53cf9d0c8: [ExtractAPI] Remove extra attributes in property declaration fragments (authored by Unique_Usman, committed by zixuw). · Explain WhyApr 4 2023, 10:02 AM
This revision was automatically updated to reflect the committed changes.
zixuw added a commit: rG32b53cf9d0c8: [ExtractAPI] Remove extra attributes in property declaration fragments.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2023, 10:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Revision Contents

PathSize
clang/
lib/
ExtractAPI/
2 lines
test/
ExtractAPI/
34 lines
32 lines
8 lines
198 lines

Diff 510847

clang/lib/ExtractAPI/DeclarationFragments.cpp

Show First 20 Lines • Show All 632 Lines • ▼ Show 20 Lines
DeclarationFragments DeclarationFragmentsBuilder::getFragmentsForObjCProperty( DeclarationFragments DeclarationFragmentsBuilder::getFragmentsForObjCProperty(
const ObjCPropertyDecl *Property) { const ObjCPropertyDecl *Property) {
DeclarationFragments Fragments, After; DeclarationFragments Fragments, After;
// Build the Objective-C property keyword. // Build the Objective-C property keyword.
Fragments.append("@property", DeclarationFragments::FragmentKind::Keyword); Fragments.append("@property", DeclarationFragments::FragmentKind::Keyword);
const auto Attributes = Property->getPropertyAttributes(); const auto Attributes = Property->getPropertyAttributesAsWritten();
zixuwUnsubmitted
Not Done
ReplyInline Actions

This is where the attributes for the property decl are retrieved.

zixuw: This is where the attributes for the property decl are retrieved.
// Build the attributes if there is any associated with the property. // Build the attributes if there is any associated with the property.
zixuwUnsubmitted
Not Done
ReplyInline Actions

Do not use comments to describe the change you've made.

zixuw: Do not use comments to describe the change you've made.
if (Attributes != ObjCPropertyAttribute::kind_noattr) { if (Attributes != ObjCPropertyAttribute::kind_noattr) {
// No leading comma for the first attribute. // No leading comma for the first attribute.
zixuwUnsubmitted
Not Done
ReplyInline Actions

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.

zixuw: I don't think this is correct. `Attributes` is simply a bitfield indicator of which attributes…
zixuwUnsubmitted
Not Done
ReplyInline Actions

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

zixuw: Please use inline comments to describe and explain the actual code logic. You can communicate…
zixuwUnsubmitted
Not Done
ReplyInline Actions

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.

zixuw: Please run the LLVM test suite after your change, and update existing tests/write new…
Unique_UsmanAuthorUnsubmitted
Done
ReplyInline Actions

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.

Unique_Usman: Thanks for the reply, my question now is, since you know more about the code than anyone else…
zixuwUnsubmitted
Not Done
ReplyInline Actions

You are close! The surrounding lines in this method retrieves and builds the fragments for the attributes.

zixuw: You are close! The surrounding lines in this method retrieves and builds the fragments for the…
Unique_UsmanAuthorUnsubmitted
Done
ReplyInline Actions

Thanks for pointing out the guide for testing.

Unique_Usman: Thanks for pointing out the guide for testing.
bool First = true; bool First = true;
Fragments.append(" (", DeclarationFragments::FragmentKind::Text); Fragments.append(" (", DeclarationFragments::FragmentKind::Text);
// Helper function to render the attribute. // Helper function to render the attribute.
auto RenderAttribute = auto RenderAttribute =
[&](ObjCPropertyAttribute::Kind Kind, StringRef Spelling, [&](ObjCPropertyAttribute::Kind Kind, StringRef Spelling,
StringRef Arg = "", StringRef Arg = "",
DeclarationFragments::FragmentKind ArgKind = DeclarationFragments::FragmentKind ArgKind =
DeclarationFragments::FragmentKind::Identifier) { DeclarationFragments::FragmentKind::Identifier) {
▲ Show 20 Lines • Show All 170 Lines • Show Last 20 Lines

clang/test/ExtractAPI/objc_category.m

Show First 20 Lines • Show All 276 Lines • ▼ Show 20 Lines "symbols": [
"accessLevel": "public", "accessLevel": "public",
"declarationFragments": [ "declarationFragments": [
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
"kind": "text", "kind": "text",
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines

clang/test/ExtractAPI/objc_id_protocol.m

Show First 20 Lines • Show All 118 Lines • ▼ Show 20 Lines "symbols": [
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "copy" "spelling": "copy"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ", " "spelling": ", "
}, },
{ {
"kind": "keyword", "kind": "keyword",
▲ Show 20 Lines • Show All 60 Lines • ▼ Show 20 Lines "symbols": [
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite" "spelling": "readwrite"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ") " "spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
▲ Show 20 Lines • Show All 99 Lines • Show Last 20 Lines

clang/test/ExtractAPI/objc_interface.m

Show First 20 Lines • Show All 428 Lines • ▼ Show 20 Lines "symbols": [
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readonly" "spelling": "readonly"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ", " "spelling": ", "
}, },
{ {
"kind": "keyword", "kind": "keyword",
▲ Show 20 Lines • Show All 249 Lines • Show Last 20 Lines

clang/test/ExtractAPI/objc_property.m

Show First 20 Lines • Show All 157 Lines • ▼ Show 20 Lines "symbols": [
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "class" "spelling": "class"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") " "spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines "symbols": [
"accessLevel": "public", "accessLevel": "public",
"declarationFragments": [ "declarationFragments": [
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
"kind": "text", "kind": "text",
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Lines "symbols": [
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "class" "spelling": "class"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") " "spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines "symbols": [
"accessLevel": "public", "accessLevel": "public",
"declarationFragments": [ "declarationFragments": [
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
"kind": "text", "kind": "text",
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Lines "symbols": [
"spelling": " (" "spelling": " ("
}, },
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "class" "spelling": "class"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") " "spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines "symbols": [
"accessLevel": "public", "accessLevel": "public",
"declarationFragments": [ "declarationFragments": [
{ {
"kind": "keyword", "kind": "keyword",
"spelling": "@property" "spelling": "@property"
}, },
{ {
"kind": "text", "kind": "text",
"spelling": " (" "spelling": " "
},
{
"kind": "keyword",
"spelling": "atomic"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "assign"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "unsafe_unretained"
},
{
"kind": "text",
"spelling": ", "
},
{
"kind": "keyword",
"spelling": "readwrite"
},
{
"kind": "text",
"spelling": ") "
}, },
{ {
"kind": "typeIdentifier", "kind": "typeIdentifier",
"preciseIdentifier": "c:I", "preciseIdentifier": "c:I",
"spelling": "int" "spelling": "int"
}, },
{ {
"kind": "text", "kind": "text",
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines