sys/threads: modify threadsinfo declaration - #454
Conversation
Summary of ChangesHello @oI0ck, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a necessary adjustment to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the function signature for threadsinfo in sys/threads.h to align with kernel changes, adding tid and flags parameters. This is a breaking change. My review focuses on the maintainability and clarity of this API modification. I have added a comment recommending the addition of documentation for the new parameters to clarify their purpose and usage, which is crucial for a public-facing API.
1b7c3e0 to
2815659
Compare
2815659 to
612c0a0
Compare
612c0a0 to
9752952
Compare
|
Breaking commits should be marked by |
|
Add kernel PR to the list list of additional PRs. |
|
|
||
|
|
||
| extern int threadsinfo(int n, threadinfo_t *info); | ||
| extern int threadsinfo(int tid, unsigned int flags, int n, threadinfo_t *info); |
There was a problem hiding this comment.
I don't like this API change. The function has become too complex. I'd rather add additional syscall wrappers to preserve backward compatibility:
/* The same as old API. Returns thread count if info==NULL? */
int threadsinfo(int n, threadinfo_t *info)
/* Adds flags argument. */
int threadsinfo3(int n, threadinfo_t *info, unsigned int flags);
/* Single thread info, not sure if we need flags here. Returns 0 on success? */
int threadinfo(int tid, threadinfo_t *info, unsigned int flags)
There was a problem hiding this comment.
There would be a symbol collision if I were to change threadsinfo() declaration here.
There was a problem hiding this comment.
To achieve what you are describing here, I'd have to rename the syscall in the kernel.
IMHO the function (or it's declaration) is not overly complex.
9752952 to
c31a681
Compare
JIRA: RTOS-1187
c31a681 to
7e67060
Compare
JIRA: RTOS-1187
Description
This pull request adjusts the function declaration to respective changes in the kernel
Types of changes
How Has This Been Tested?
Checklist:
Special treatment