lfs_crc should be static if LFS_CRC is defined

[DvdGiessen]

If LFS_CRC is defined, we define a wrapper function in the header file itself instead of referring to a function implemented later in lfs_utils.c. In that case we should declare that wrapper function to be static so that we don't end up with multiple definitions of the lfs_crc function if the header file (as is quite usual) gets included in multiple compile units.

Additionally it can also be declared inline to match the other functions. And, there was a missing semicolon meaning one would have to include an extra semicolon in their #define macro to make everything compile without errors. Adding in the semicolon won't break any existing users that already included it themselves.

[geky-bot]

Tests passed ✓, Code: 17116 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17116 B (+0.0%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2432/2594 lines (+0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1279/1610 branches (-0.0%)
Threadsafe	17968 B (+0.0%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17188 B (+0.0%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18780 B (+0.0%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17896 B (+0.0%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

[bmcdonnell-fb]

I think a more consistent approach would be to get rid of the #ifdef LFS_CRC in the header (i.e. always declare the lfs_crc() function), and let the user implement the lfs_crc() function in their own source files. I don't think this should be a static inline function.

[DvdGiessen]

I kept it in the same style as LFS_MALLOC and LFS_FREE which also use this pattern, so it's consistent between all these macro's / function signatures.

Changing how one can provide their own CRC function, for example in the way you described, might perhaps also be a improvement but one I think that would best be discussed in a separete PR since that would mean a more significant change in the interface which I assume was designed and discussed at the time this feature was added.

I don't have an opinion on what would in the end be the superior interface; but perhaps others do. For that reason I would prefer that a simpler fix (like this one, but any other that solves the stated issue is fine too) would be considered for merging first, since it doesn't change the existing interface and thus can presumably be merged with less discussion so current use cases are no longer blocked. That still leaves room for a bigger change that changes the interface later.

[bmcdonnell-fb]

@DvdGiessen, good points, and it does have me questioning whether my suggestion is better overall. However...

I kept it in the same style as LFS_MALLOC and LFS_FREE which also use this pattern, so it's consistent between all these macro's / function signatures.

What you have here is not quite the same as those, since lfs_malloc() and lfs_free() are declared static inline, outside of the #ifs.

[geky]

Wow! Who let this get merged (it was me).

Thanks @DvdGiessen for the fix, will bring this in on the next patch release. Sorry about the late response.

---

@bmcdonnell-fb I agree with @DvdGiessen, feel free to create an issue or PR if you think it needs to change.

But the reason for the static inline is that it forces user provided impls to have the correct type. Is there a real risk of mismatched types breaking things? Not sure. But it doesn't hurt anything (unless you botch it like here).

It at least avoids a warning pile if user's malloc returns uint8_t*.

W.r.t. static inline vs no static inline, I think that's more an implementation detail. If lfs_util.h ever included its own heap impl (maybe for say some fixed number of files), the non-user lfs_malloc would probably not be static inline.

[bmcdonnell-fb]

@geky

W.r.t. static inline vs no static inline, I think that's more an implementation detail.

But that's all this PR does...?

Also, git grep -w lfs_crc shows me that it's called in several places. Why would you want it to be inline?

[geky]

I'm assuming no one is putting an entire CRC implementation directly inside the LFS_CRC macro. I would expect LFS_CRC to just be the name of your own my_crc function, which would make lfs_crc a small inlinable function.

If we wanted to be perfectly consistent, we could do this:

#ifndef LFS_CRC
uint32_t lfs_crc_(uint32_t crc, const void *buffer, size_t size);
#endif

static inline uint32_t lfs_crc_(uint32_t crc, const void *buffer, size_t size) {
    #ifdef LFS_CRC
    return LFS_CRC(crc, buffer, size);
    #else
    return lfs_crc_(crc, buffer, size);
    #endif
}

But that's a bit silly.

[bmcdonnell-fb]

@geky, I see what you mean. However, FWIW, the intent and usage seems clearer to me from your "perfectly consistent" example.

If you merge this as is, might it be worth adding a comment to indicate, as you said,

I would expect LFS_CRC to just be the name of your own my_crc function

?

OK, that's enough from me on this. Thanks for all your work on the project.

[geky]

Bringing this in, hopefully better late than never, thanks for the PR!