Skip to content

Conversation

@toastdb
Copy link

@toastdb toastdb commented Sep 18, 2025

There are a few sign conversion warnings when building with -Wsign-conversion in lfs_util.h that I'd like to resolve.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17112 B (+0.0%) 1448 B (+0.0%) 812 B (+0.0%) Lines 2435/2595 lines (-0.0%)
Readonly 6230 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1283/1616 branches (+0.0%)
Threadsafe 17964 B (+0.0%) 1448 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17184 B (+0.0%) 1448 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18776 B (+0.0%) 1752 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17924 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky geky added the lint label Sep 23, 2025
@geky
Copy link
Member

geky commented Sep 23, 2025

Hi @toastdb, thanks for creating a PR.

Two concerns:

  1. Compiling lfs.c with -Wsign-conversion results in 104 warnings. What is the use case for compiling lfs_util.h differently?

    If -Wsign-conversion can't be adopted in littlefs's Makefile this is is high-risk for regressing in the future.

  2. This risks hiding legitimate type mismatches between, for example, uint32_t and __builtin_ctz, __builtin_ctzl, __builtin_ctzll, etc. due to the bluntness of C's typecasts.

@toastdb
Copy link
Author

toastdb commented Sep 23, 2025

Hi @geky,

  1. We compile our source with stricter flags than 3rd party code. Because lfs.h is included and brings in lfs_util.h the warnings pop up. I did attempt to enable the flag for the whole littlefs repo and saw resolving that would be pretty intrusive for something nobody asked for.
  2. This seems like a valid concern and I'm happy to close the PR.

@toastdb toastdb closed this Sep 23, 2025
@geky
Copy link
Member

geky commented Sep 23, 2025

We compile our source with stricter flags than 3rd party code. Because lfs.h is included and brings in lfs_util.h the warnings pop up.

Hmm, I can see where this would be annoying.

It's not the cleanest solution, but would wrapping the include with a pragma work?

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wsign-conversion"
#include "lfs.h"
#pragma GCC diagnostic pop

Another option is copying lfs_util.h, applying the above patch, and using -DLFS_CONFIG=my_util.h. lfs_util.h is intended to be replaceable, though not quite for this reason.

@toastdb
Copy link
Author

toastdb commented Sep 25, 2025

I appreciate the tips but we've got them in an ignore list, we were just trying to decrease the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants