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

Add an alternative way to override LFS_MALLOC etc #1004

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

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Jul 10, 2024

With the existing method, (-DLFS_MALLOC=my_malloc) users often had to use compiler options like -include, which was not so portable.
This change introduces another way to provide partial overrides of lfs_util.h using a user-provided header.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17064 B (+0.0%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17064 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Lines 2394/2574 lines (-0.0%)
Readonly 6194 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1245/1584 branches (+0.0%)
Threadsafe 17924 B (+0.0%) 1440 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17124 B (+0.0%) 1440 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18760 B (+0.0%) 1744 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17748 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Sep 17, 2024

Hi @yamt, thanks for creating a PR, and sorry about such a late response.

This is a clever naming solution, I was stuck and couldn't think of a name since LFS_CONFIG was taken. These should have probably been named LFS_CONFIG/LFS_UTIL but that's with hindsight.

Will bring this in on the next minor/patch release (is it unreasonable to slip this in on a patch release?)

lfs_util.h Outdated Show resolved Hide resolved
@yamt
Copy link
Contributor Author

yamt commented Sep 18, 2024

the ci failure seems unrelated to this PR.

@yamt
Copy link
Contributor Author

yamt commented Sep 18, 2024

the ci failure seems unrelated to this PR.

#1026

@geky geky changed the base branch from master to devel September 24, 2024 17:25
With the existing method, (-DLFS_MALLOC=my_malloc)
users often had to use compiler options like -include, which
was not so portable.
This change introduces another way to provide partial overrides of
lfs_util.h using a user-provided header.
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17064 B, Stack: 1440 B, Structs: 812 B
Code Stack Structs Coverage
Default 17064 B 1440 B 812 B Lines 2394/2574 lines
Readonly 6194 B 448 B 812 B Branches 1245/1584 branches
Threadsafe 17924 B 1440 B 820 B Benchmarks
Multiversion 17124 B 1440 B 816 B Readed 29369693876 B
Migrate 18760 B 1744 B 816 B Proged 1482874766 B
Error-asserts 17748 B 1432 B 812 B Erased 1568888832 B

@marcosgabrielserrano
Copy link

👀

@BenBE
Copy link

BenBE commented Nov 15, 2024

Doesn't this effectively make LFS_CONFIG and LFS_DEFINES be the same (except for the latter also defining the default utils too)?

A better implementation would be to have LFS_CONFIG work just like now, but allow the user config to (un)define LFS_SKIP_UTILS to emulate both behaviors. lfs_utils.h on the other hand should be made aware that all of the macros it defines, may have been provided by the custom user header already.

@geky
Copy link
Member

geky commented Nov 18, 2024

I think it's a bit too late to be bikeshedding this. The only reason this isn't already merged is delays on my end in making a minor release (sorry! in hindsight the upcoming minor release should have been split into multiple releases).

Though discussion can influence future API changes.


A better implementation would be to have LFS_CONFIG work just like now, but allow the user config to (un)define LFS_SKIP_UTILS to emulate both behaviors.

This is an interesting and clever workaround, but thinking from the perspective of a new user, you're asking them to do 2 things (define LFS_CONFIG and (un)define LFS_SKIP_UTILS) instead of 1 (define LFS_DEFINES). I suspect LFS_DEFINES will be more common for users who aren't doing a full OS integration, so having it require more steps isn't the greatest.

I'm not against dropping LFS_CONFIG on a later major API change if LFS_DEFINES (renamed -> LFS_CONFIG?) can fully replace it.

lfs_utils.h on the other hand should be made aware that all of the macros it defines, may have been provided by the custom user header already.

This gets a bit tricky for things like the type definitions. Though maybe we require all user-provided types to also be macros?

#ifdef LFS_SIZE_T
typedef LFS_SIZE_T lfs_size_t
#else
#include <stdint.h>
typedef uint32_t lfs_size_t
#endif

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