nuttxpr commented on PR #14094: URL: https://github.com/apache/nuttx/pull/14094#issuecomment-2406688381
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This Pull Request seems to **mostly** meet the NuttX PR requirements, but could use some improvements for clarity and completeness. **Strengths:** * **Summary:** Clearly states the "what" of the change: More information will be dumped on memory manager failures if a config flag is enabled. * **Testing:** Specifies CI testing was done, along with the architecture and board used. **Areas for Improvement:** * **Summary:** * **Why:** Explain the reason behind needing more detailed dumps. What kind of diagnostics does this aid in mass-production? * **How:** Be more specific about the "more information" being added. What data will be included in the dump that wasn't there before? * **Flush Log:** Briefly explain why flushing the log before a coredump is important. * **Issues:** Link any related NuttX issues if this PR addresses them. * **Impact:** * **New/Changed Feature:** While technically correct, be more explicit. State if this is a bug fix, improvement to an existing feature (dumping), or a completely new feature. * **User Impact:** Will users need to enable the `CONFIG_MM_DUMP_DETAILS_ON_FAILURE` flag to see the change? If so, mention it. * **Build Impact:** Unlikely, but if the added logging has any potential impact on build times (especially for resource-constrained targets), mention it. * **Other Impacts:** All marked as "NO" without explanation. While likely accurate, consider adding a brief "N/A" or a sentence like "No other anticipated impacts" for completeness. * **Testing:** * **Build Host:** Provide details about the CI environment (OS, compiler used for the host). * **Logs:** Instead of just stating "testing logs", either provide snippets of the relevant log sections or explain what differences were observed in the logs before and after the change. **In short, be as specific and detailed as possible while remaining concise. This helps reviewers understand the PR's value and potential ramifications quickly.** -- 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
