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

system/nxplayer/nxrecorder: move apb buffer instance to stack #3023

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Mar 10, 2025

Summary

system/nxplayer/nxrecorder: move apb buffer instance to stack

move apb buffer instance to stack to avoid alloc buffer from heap

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

@nuttxpr
Copy link

nuttxpr commented Mar 10, 2025

[Experimental Bot, please feedback here]

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

Here's a breakdown of the missing information:

  • Insufficient Summary: While the summary explains what was changed, it doesn't fully explain why. Why is allocating from the heap problematic in this specific case? What benefits does moving to the stack provide (e.g., performance improvement, deterministic memory usage, avoiding heap fragmentation)? What is "apb buffer"? More context is needed. It also lacks any mention of related issues.

  • Missing Impact Assessment: Simply stating "N/A" is insufficient. Even seemingly small changes can have unforeseen impacts. Consider these points:

    • Impact on hardware: Does this change affect any specific architectures or boards? It's unlikely, but needs explicit confirmation.
    • Impact on compatibility: Does this change affect any existing applications using the nxrecorder? Again, unlikely but needs to be addressed.
    • Impact on security: While unlikely in this case, always consider potential security implications. For example, does moving the buffer to the stack introduce any risks related to stack overflow? Explicitly stating "NO" with a brief justification is better than "N/A".
  • Inadequate Testing: "ci-check" is not sufficient. While CI is important, the PR needs to demonstrate specific tests performed locally that validate the change. What functionality was tested? What were the expected results? Providing actual log snippets (even if they're short and show expected behavior) is crucial for reviewers to understand the testing process. Furthermore, the details of the build host and target(s) are missing.

In short: The PR needs more detail and explicit confirmation (even if negative) for several impact categories. The testing section requires significant improvement to show how the changes were validated.

@acassis acassis requested a review from tmedicci March 14, 2025 13:37
move apb buffer instance to stack to avoid alloc buffer from heap

Signed-off-by: chao an <[email protected]>
Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @anchao :-)

@cederom cederom requested review from acassis, jerpelea and lupyuen March 25, 2025 16:25
@xiaoxiang781216 xiaoxiang781216 merged commit 6d0a039 into apache:master Mar 25, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants