Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function header description consistency #478

Merged
merged 2 commits into from
Apr 26, 2023
Merged

Conversation

tammyleino
Copy link
Collaborator

Updated function header descriptions formatting to be consistent.
Signed-off-by: Tammy Leino tammy.leino@siemens.com

@tammyleino
Copy link
Collaborator Author

The purpose of this PR is to see a sample of the changes I am proposing for all the API function headers.

@arnopo arnopo requested review from edmooring and arnopo April 11, 2023 12:02
@arnopo
Copy link
Collaborator

arnopo commented Apr 11, 2023

The purpose of this PR is to see a sample of the changes I am proposing for all the API function headers.

Look good to go with this function header formatting.
Thanks!

@tammyleino tammyleino force-pushed the issue_477 branch 2 times, most recently from e74109c to d8850d1 Compare April 13, 2023 15:16
@tammyleino
Copy link
Collaborator Author

@arnopo Is there a way to override the failure of the checker not recognizing the word "doesn't"?

@arnopo
Copy link
Collaborator

arnopo commented Apr 14, 2023

@arnopo Is there a way to override the failure of the checker not recognizing the word "doesn't"?

The issue come from the checkpatch script
It has not be updated since a while
I entered an issue #483

In the meantime, just ignore the issue

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems correct but I need to apply it for a more complete review.
in a first step could you split in 2 commits, a separate one to fix the Doxyfile?

Doxyfile Show resolved Hide resolved
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @arnopo is there a way to have checkpatch look for this automatically?

@arnopo
Copy link
Collaborator

arnopo commented Apr 16, 2023

Looks good to me. @arnopo is there a way to have checkpatch look for this automatically?

you mean check the header syntax for doxygen?
When no more warning will be reported, we can perhaps run the doc generation and check for some warnings or errors. I don't find yet easy way to check the documentation update on a PR.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!
I have few comments.
Some can be addressed in a next step.

lib/include/openamp/rpmsg.h Outdated Show resolved Hide resolved
lib/include/openamp/version.h Outdated Show resolved Hide resolved
lib/remoteproc/rsc_table_parser.c Outdated Show resolved Hide resolved
lib/remoteproc/rsc_table_parser.c Outdated Show resolved Hide resolved
lib/virtio/virtqueue.c Outdated Show resolved Hide resolved
* @param data Payload of the message
* @param len Length of the payload
*
* @return Number of bytes it has sent or negative error value on failure.
*/
static inline int rpmsg_send(struct rpmsg_endpoint *ept, const void *data,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems that doxygen does not generate the documentation for static function. I don't know if there is a way to force it.... do you have a simple solution?

An alternative (which would get advantage to improve the support of RUST client) would be an implementation similar to
this proposal:
OpenAMP/libmetal@ef0a476

For this release we can just enter an issue to fix that later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnopo I think we should fix it according to the ticket you quote, but maybe not for this release since I foresee a lot of back and forth with that solution. I can set EXTRACT_STATIC to YES to resolve for Doxygen, and I will open a ticket for additional changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, Let treat "static" declaration issue in a next step and get this one for this release.

Updated function header descriptions formatting to be consistent.
Signed-off-by: Tammy Leino <tammy.leino@siemens.com>
Set tabs to 8 spaces and enabled doc generation for static functions.
Signed-off-by: Tammy Leino <tammy.leino@siemens.com>
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arnopo arnopo added this to the Release V2023.04 milestone Apr 17, 2023
@arnopo arnopo requested a review from edmooring April 24, 2023 07:51
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go.

@arnopo arnopo merged commit 1c29b15 into OpenAMP:main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants