Use getPropertyAttributesAsWritten instead of getPropertyAttributes
to get property attributes actually specified in the source code.
Resolves issue #61478.
Details
Diff Detail
Unit Tests
Event Timeline
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. | |
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. 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.
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. |
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. |
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.
Hi Usman, as zixuw said this doesn't look correct as-is. Are you basing this work on a github issue?
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?
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
changed the getPropertyAttributes() to getPropertyAttributesAsWritten() which is the function for getting the property declared by the user.
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.
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/.
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.
Hello @zixuw, kinly review it sir, I also updated the test as you made mentioned sir. Thank you sir.
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
If this is okay, kindly help to make the commit sir. My email is usmanakinyemi202@gmail.com
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.
Kindly help to make the commit sir. My email is usmanakinyemi202@gmail.com
I hope to jump on another ExtractAPI issues. Thanks.
This is where the attributes for the property decl are retrieved.