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

add test for longjump with 0 as return value #2725

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

realprocrastinator
Copy link

Summary

This is a re-submitted patch reverted @#2720 due to a dependency issue with PR apache/nuttx#14152.

In this PR, we add a testcase specifically checking the behaviour of setjmp() after passing 0 as the return value (aka the second argument) to the longjmp().

Impact

Results in the incorrect behaviour which doesn't meet the POSIX specification.

Testing

ostest

@nuttxpr
Copy link

nuttxpr commented Oct 15, 2024

[Experimental Bot, please feedback here]

Let's analyze this Pull Request (PR) against the NuttX requirements:

Strengths:

  • Clear Summary: The PR explains the "why" (re-submission, dependency fix) and the "what" (testcase for setjmp/longjmp).
  • Impact Acknowledged: The PR identifies an incorrect behavior violating POSIX, which is crucial information.
  • Testing Mentioned: The use of "ostest" implies testing was done, though more details would strengthen this.

Areas for Improvement:

  • Functional Change Details: The summary should be more specific. How does the testcase work? What exact code is modified?
  • Impact Specificity:
    • User Impact: Will this change how applications using setjmp/longjmp behave? If so, how?
    • Other Impacts: Most are marked "NO" without explanation. While likely accurate, brief justifications (e.g., "No new hardware features used") enhance clarity.
  • Testing Details:
    • Insufficient Logs: "ostest" alone isn't enough. Provide snippets showing the failing behavior before your change and the correct behavior after.
    • Environment Context: List the specific build host and target(s) used for verification. This helps reviewers reproduce results.

Verdict:

The PR is on the right track but needs more detail to meet NuttX standards. Address the "Areas for Improvement" to make it easier for reviewers to assess and merge.

Signed-off-by: Gao Jiawei <gaojiawei@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 30215c2 into apache:master Oct 15, 2024
36 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.

3 participants