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

libc: modify open,close... to _NX_OPEN,_NX_CLOSE #14651

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Nov 5, 2024

Summary

libc: modify open,close... to _NX_OPEN,_NX_CLOSE

Impact

None

Testing

None

@github-actions github-actions bot added Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 5, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 5, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details. Specifically:

  • Insufficient Summary: "modify open,close... to _NX_OPEN,_NX_CLOSE" is vague. Why was this change necessary? What problem does it solve? What is the functional impact? How does using _NX_ prefixed functions change the behavior?
  • Incomplete Impact Assessment: Claiming "None" for impact is highly unlikely. At minimum, this change affects compatibility. Does it break existing code? Does it improve portability? The impact assessment needs to be thoroughly considered and documented.
  • Missing Testing Information: "None" for testing is unacceptable. Every PR requires testing. What platforms was this tested on (host and target)? What tests were run? What were the results? Providing "before" and "after" logs, even if they appear identical, demonstrates that testing was performed.

This PR needs significant revision before it can be considered. The author needs to provide specific details in each section to demonstrate that the change is well-motivated, thoroughly tested, and doesn't introduce unintended consequences.

@xiaoxiang781216
Copy link
Contributor

@W-M-R Please fix:

Error: /home/runner/work/nuttx/nuttx/nuttx/libs/libc/lzf/lzf_d.c:5:80: error: Long line found

@acassis
Copy link
Contributor

acassis commented Nov 7, 2024

@W-M-R I don't remember why these changes were made. Is it necessary to separate kernel from user space APIs?

@W-M-R W-M-R closed this Nov 11, 2024
@W-M-R W-M-R deleted the modify_file_name branch November 11, 2024 03:48
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

#14651

@W-M-R I don't remember why these changes were made. Is it necessary to separate kernel from user space APIs?

Yes, kernel mode and user mode need to use different APIs

@W-M-R W-M-R restored the modify_file_name branch November 11, 2024 04:04
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

Sorry, I accidentally disabled this patch.

@W-M-R W-M-R reopened this Nov 11, 2024
@W-M-R W-M-R force-pushed the modify_file_name branch 2 times, most recently from eff7e26 to 4c560a1 Compare November 11, 2024 04:25
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

The automatic test failed. Please give me some time to check it carefully.

@xiaoxiang781216
Copy link
Contributor

need rebase to fix the conflict

@W-M-R W-M-R force-pushed the modify_file_name branch 2 times, most recently from 0ef85f0 to e137334 Compare November 13, 2024 03:02
Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants