Skip to content

Conversation

@OceanfromXiaomi
Copy link
Contributor

@OceanfromXiaomi OceanfromXiaomi commented Sep 18, 2024

Summary

Add g_ prefix to can_dlc_to_len and len_to_can_dlc.

Impact

Add g_ prefix to can_dlc_to_len and len_to_can_dlc to follow NuttX coding style conventions for global symbols, improving code readability and maintainability.

Testing

detail: Add g_ prefix to can_dlc_to_len and len_to_can_dlc to
follow NuttX coding style conventions for global symbols,
improving code readability and maintainability.

Signed-off-by: zhaohaiyang1 <zhaohaiyang1@xiaomi.com>
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

Yes, the PR appears to meet NuttX requirements.

Here's a breakdown:

Meets Requirements:

  • Summary: Clearly states the "why" (coding style) and "what" (adding 'g_' prefix to functions).
  • Impact:
    • Accurately identifies the limited scope of the change (coding style only).
    • Correctly states there's no user, build, hardware, security, or compatibility impact.
    • Could benefit from explicitly stating if documentation needs an update.
  • Testing:
    • While the template is provided, the lack of actual test logs is a significant omission. This needs to be filled in.

Recommendations for Improvement:

  • Testing: Provide specific test logs. This is crucial for reviewers to understand how the change was validated. Include:
    • Commands used to build and run tests.
    • Relevant output snippets that demonstrate the change's effect (or lack thereof, since it's stylistic).
  • Impact - Documentation: Specify if the coding style change requires a corresponding update to any documentation (e.g., coding guidelines).

Concise Feedback:

The PR summary and impact assessment meet NuttX requirements. However, providing detailed test logs is essential for a complete and reviewable PR.

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 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit f76c2ed into apache:master Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants