nuttxpr commented on PR #16280: URL: https://github.com/apache/nuttx/pull/16280#issuecomment-2836126185
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some sections could be expanded for clarity. Specifically, the Testing section needs actual test logs and details of the testing environment. Here's a breakdown of why it *mostly* meets the requirements and suggestions for improvement: **Strengths:** * **Clear Summary:** The summary clearly explains the problem (POSIX violation), the proposed solution, and the rationale. The link to the POSIX standard is helpful. * **Impact Assessment:** The impact section acknowledges the breaking change and its implications. It also justifies the change despite the breakage. * **Justification for Breaking Change:** The justification for the breaking change is strong, rooted in adherence to the POSIX standard and NuttX's own "Inviolable Principles." **Weaknesses and Suggestions:** * **Missing Testing Details:** The "Testing" section is empty. You *must* provide: * Specific build host details (OS, CPU architecture, compiler version). * Specific target details (architecture, board, configuration). * Actual log output before and after the change, demonstrating the corrected behavior. Show how passing 0 now returns the previous mask and doesn't modify logging. Ideally, also show a case where a non-zero mask *does* change the logging level as expected. * **Impact - User:** While the impact on the user is described, it could be more specific. *Which* applications might be affected? Are these internal to NuttX or external user applications? Providing examples of code that would need to be changed would be very helpful. * **Impact - Documentation:** Does this change require updating any NuttX documentation? If so, has the documentation been updated in this PR, or is a separate PR planned? Even if no documentation changes are needed, explicitly stating "NO" for documentation impact would be good for clarity. * **Impact - Compatibility:** The compatibility impact is touched upon, but could be strengthened. Mentioning specific versions of other POSIX systems (e.g., Linux, macOS) and how their `setlogmask` behaves would reinforce the justification for the change. **Example Testing Section Improvement:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0 * Target(s): sim:qemu-arm Testing logs before change: ``` (Illustrative Example) Calling setlogmask(0) unexpectedly disables all logging. Previous log messages... (No further log messages appear) ``` Testing logs after change: ``` (Illustrative Example) Calling setlogmask(0) returns the current log mask (e.g., 0xFF). Previous log messages... Subsequent log messages continue to appear at the previous level. Calling setlogmask(LOG_ERR) restricts logging to errors only. Previous log messages... (Only error messages appear) ``` ``` By adding these details and strengthening the slightly weaker sections, this PR will be much more likely to be accepted. Remember, thoroughness and clarity are key to a successful PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
