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

fs cases: fix compile error #2905

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

kernel/fs/cases/fs_stream_test.c:279:70: error: format ‘%zi’ expects argument of type ‘signed size_t’, but argument 4 has type ‘int’ [-Werror=format=]
  279 |           syslog(LOG_ERR, "len = %zi != return value from fwrite = %zi",
      |                                                                    ~~^
      |                                                                      |
      |                                                                      long int
      |                                                                    %i
  280 |                  len, ret);
      |                       ~~~
      |                       |
      |                       int
kernel/fs/cases/fs_stream_test.c:311:69: error: format ‘%zi’ expects argument of type ‘signed size_t’, but argument 4 has type ‘int’ [-Werror=format=]
  311 |           syslog(LOG_ERR, "len = %zi != return value from fread = %zi",
      |                                                                   ~~^
      |                                                                     |
      |                                                                     long int
      |                                                                   %i
  312 |                  len, ret);
      |                       ~~~
      |                       |
      |                       int

Impact

fix compiler warning

Testing

ci

kernel/fs/cases/fs_stream_test.c:279:70: error: format ‘%zi’ expects argument of type ‘signed size_t’, but argument 4 has type ‘int’ [-Werror=format=]
  279 |           syslog(LOG_ERR, "len = %zi != return value from fwrite = %zi",
      |                                                                    ~~^
      |                                                                      |
      |                                                                      long int
      |                                                                    %i
  280 |                  len, ret);
      |                       ~~~
      |                       |
      |                       int
kernel/fs/cases/fs_stream_test.c:311:69: error: format ‘%zi’ expects argument of type ‘signed size_t’, but argument 4 has type ‘int’ [-Werror=format=]
  311 |           syslog(LOG_ERR, "len = %zi != return value from fread = %zi",
      |                                                                   ~~^
      |                                                                     |
      |                                                                     long int
      |                                                                   %i
  312 |                  len, ret);
      |                       ~~~
      |                       |
      |                       int

Signed-off-by: zhangshoukui <[email protected]>
…long unsigned int', but argument 3 has type 'size_t'

Signed-off-by: Xiang Xiao <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Dec 18, 2024

[Experimental Bot, please feedback here]

Yes, this PR likely meets the NuttX requirements, though it could be slightly improved.

Strengths:

  • Clear Summary: The summary identifies the problem (compiler warnings related to format specifiers) and the affected file. The error messages themselves are helpful context.
  • Concise Impact: Clearly states the impact is fixing a compiler warning.
  • Testing Indication: "ci" suggests Continuous Integration testing was performed, which is generally good.

Weaknesses/Areas for Improvement:

  • Missing Details in Summary: While the error messages are present, a brief explanation of why the change was made (e.g., "Incorrect format specifier %zi used for int type, changed to %i") would enhance clarity.
  • Vague Testing: "ci" is insufficient. While CI is good, the PR should specify:
    • Build Host(s): The CI environment details (OS, compiler, etc.) are missing.
    • Target(s): Which architectures/boards were tested in CI?
    • Logs (or their absence): Ideally, relevant snippets of the CI logs (before and after) showing the warning disappearing should be included. If the logs are excessively long, a statement like "CI logs show successful compilation without the mentioned warnings on [list of architectures]" would suffice.

Recommendation:

Add the missing details mentioned above, especially the testing information. While the PR is likely acceptable as-is, providing more context will make it easier for reviewers to understand and approve the change.

Copy link

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

@tmedicci
Copy link
Contributor

Thanks, @xiaoxiang781216 !

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.

5 participants