Skip to content

fs cases: fix compile error #2900

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

Closed
wants to merge 2 commits into from
Closed

Conversation

Zhangshoukui
Copy link
Contributor

@Zhangshoukui Zhangshoukui commented Dec 18, 2024

Summary

Fix compile error:

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

buld

Testing

pass 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 <zhangshoukui@xiaomi.com>
@nuttxpr
Copy link

nuttxpr commented Dec 18, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. While it identifies a compile error, it provides no context about the change that caused the error or what was done to fix it. It also lacks critical information for reviewers.

Here's why it's insufficient and what's missing:

  • Summary: "compile error" is not a helpful summary. It needs to describe the reason for the change (fixing a compile error), what part of the code was changed (the format string in fs_stream_test.c), and how the change addresses the error (presumably changing %zi to %i or changing the argument type). There's no mention of a related NuttX issue.
  • Impact: Again, simply stating "compile error" isn't enough. While the immediate impact is a build failure, the PR needs to describe the root cause of the error. Was existing code broken by a recent change? Was new code introduced with incorrect types? This helps understand the potential ripple effects. All other impact categories are unaddressed.
  • Testing: Completely missing. The PR needs to indicate the build host and target used for testing, along with logs from before the change (showing the compile error) and after (demonstrating the fix). This validates that the change actually resolves the problem.

Example of a better summary:

Fixes a compile error in fs_stream_test.c caused by using the incorrect format specifier %zi for an int argument in syslog. The format specifier has been corrected to %i to match the argument type, resolving the build failure. No related NuttX issue found.

Example of a better impact assessment:

  • Is new feature added? Is existing feature changed?: No. Bug fix.
  • Impact on user: NO.
  • Impact on build: YES. The code now compiles correctly. Previously, it would fail with the reported error.
  • Impact on hardware: NO.
  • Impact on documentation: NO.
  • Impact on security: NO.
  • Impact on compatibility: NO.
  • Anything else to consider: NO.

Example of basic testing information:

  • Build Host(s): Linux, x86_64, GCC 12.2
  • Target(s): qemu-x86_64

(Logs before showing the error, logs after showing successful compilation)

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

Looks like GH detected conflict that needs to be resolved before merge? :-)

@xiaoxiang781216
Copy link
Contributor

@Zhangshoukui please rebase the patch to fix the conflict.

@xiaoxiang781216
Copy link
Contributor

let's track this pr instead: #2905

@Zhangshoukui Zhangshoukui deleted the fs_ branch April 8, 2025 07:07
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.

4 participants