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

lfs_fs_raw* functions should be static #884

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

DvdGiessen
Copy link
Contributor

@DvdGiessen DvdGiessen commented Oct 23, 2023

Two new functions introduced in 2.6.0 (lfs_fs_mkconsistent, 259535e) and 2.8.0 (lfs_fs_grow, 23505fa) have internal "raw" functions which are missing the static keyword.

This was causing build failures in our builds in the MicroPython project when updating to the latest LittleFS, because we're building with -Werror,-Wmissing-prototypes,-Wmissing-declarations. See here and here for the build output.

This PR adds the static keyword to these functions to fix these, similar to how all other internal functions in LittleFS are defined.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 16820 B (-0.1%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)
Code Stack Structs Coverage
Default 16820 B (-0.1%) 1448 B (+0.0%) 800 B (+0.0%) Lines 2357/2533 lines (+0.0%)
Readonly 6130 B (+0.0%) 448 B (+0.0%) 800 B (+0.0%) Branches 1202/1528 branches (+0.0%)
Threadsafe 17688 B (-0.2%) 1448 B (+0.0%) 808 B (+0.0%) Benchmarks
Multiversion 16884 B (-0.1%) 1448 B (+0.0%) 804 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18500 B (-0.1%) 1752 B (+0.0%) 804 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17480 B (-0.1%) 1440 B (+0.0%) 800 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Oct 24, 2023

Hi @DvdGiessen, thanks for this! Looks like quite an oversight.

Will bring this in next patch release.

This warning is useful for catching the easy mistake of missing the
keyword static on functions intended to be internal-only.

Missing the static keyword risks symbol polution and misses potential
compiler optimizations.

This is an interesting warning, while useful for libraries such as
littlefs, it's perfectly valid C to not predeclare all functions, and
common in final application binaries.

Relatedly, this warning is re-disabled for the test/bench runner. There
may be a better way to organize the CFLAGS, maybe into separate
LIB/RUNNER CFLAGS, but I'll leave this to future work if our CFLAGS grow
more complicated.

This was motivated by non-static internal-only functions leaking into a
release. Found and fixed by DvdGiessen.
@geky
Copy link
Member

geky commented Oct 24, 2023

I've gone ahead and added -Wmissing-prototypes into our Makefile (8f3f32d), it seems like a useful warning. This should now be caught by the CI here and prevented in the future.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 16820 B (-0.1%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)
Code Stack Structs Coverage
Default 16820 B (-0.1%) 1448 B (+0.0%) 800 B (+0.0%) Lines 2357/2533 lines (+0.0%)
Readonly 6130 B (+0.0%) 448 B (+0.0%) 800 B (+0.0%) Branches 1202/1528 branches (+0.0%)
Threadsafe 17688 B (-0.2%) 1448 B (+0.0%) 808 B (+0.0%) Benchmarks
Multiversion 16884 B (-0.1%) 1448 B (+0.0%) 804 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18500 B (-0.1%) 1752 B (+0.0%) 804 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17480 B (-0.1%) 1440 B (+0.0%) 800 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky geky changed the base branch from master to devel October 31, 2023 18:26
@geky geky merged commit c733d9e into littlefs-project:devel Oct 31, 2023
93 checks passed
@geky
Copy link
Member

geky commented Oct 31, 2023

Should be merged now, thanks again for this!

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.

3 participants