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

stdio/va_format: move non-standard structure va_format to nuttx/streams.h #13575

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

Donny9
Copy link
Contributor

@Donny9 Donny9 commented Sep 23, 2024

Summary

stdio/va_format: move non-standard structure va_format to nuttx/streams.h

Impact

N/A

Testing

local compile

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Sep 23, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 23, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX Requirements.

Missing Information:

  • Summary:
    • Lacks a clear explanation of why the change is necessary. Is it a bug fix, code cleanup, performance improvement, etc.?
    • Needs more details on how the change works. Simply stating that a structure is being moved is insufficient. Explain the reasoning and implications.
  • Impact:
    • While marking all sections as "N/A" might be technically correct, it's crucial to elaborate and justify why there's no impact in any area. For instance, even moving a structure could have implications on backward compatibility.
  • Testing:
    • "Local compile" is insufficient testing. Provide specific details:
      • Build Host(s) OS, CPU architecture, compiler version.
      • Target(s): Architecture (e.g., simulator, real hardware), board, configuration.
    • Include relevant testing logs before and after the change to demonstrate the problem being solved or the functionality being added.

Recommendations:

  1. Expand the Summary: Explain the motivation for the change and its technical details comprehensively.
  2. Address Impact Carefully: Even if there's no apparent impact, explain why. For example, "This change only affects internal code structure and has no impact on the public API, build process, or hardware compatibility."
  3. Provide Detailed Testing Information: List specific build hosts and targets, and include relevant log snippets to demonstrate the change's effect.

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please add commit messages which describe the change and why is it needed

@cederom
Copy link
Contributor

cederom commented Sep 23, 2024

Yes we need increase PR and git commit descriptions quality (both need to be self explanatory).. bot was created to help.. you can .. wait was there this nice copy-paste section where all sections were filled in already before @lupyuen ? :-)

@Donny9
Copy link
Contributor Author

Donny9 commented Sep 23, 2024

please add commit messages which describe the change and why is it needed

I have explained clearly enough in the commit message. Please read carefully. va_format is not a standard structure and cannot be placed in standard stdio files. @jerpelea @cederom

@pkarashchenko
Copy link
Contributor

Please provide link to nuttx-apps PR as well.

@Donny9
Copy link
Contributor Author

Donny9 commented Sep 24, 2024

Please provide link to nuttx-apps PR as well.

apache/nuttx-apps#2600 Done~

@xiaoxiang781216
Copy link
Contributor

@Donny9 please fix the build error

github/workspace/sources/apps/system/adb/adb_main.c: In function 'adb_log_impl':
Error: /github/workspace/sources/apps/system/adb/adb_main.c:46:20: error: storage size of 'vaf' isn't known
   46 |   struct va_format vaf;
      |                    ^~~

@pkarashchenko
Copy link
Contributor

Need to re-push the change to get actual apps version in the build

@xiaoxiang781216 xiaoxiang781216 merged commit 6526405 into apache:master Sep 27, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants