Fix release build warning ('lfs_mlist_isopen' defined but not used)

[geky]

Hi @bmcdonnell-fb, thanks for creating a PR.

I don't think this works in the case where LFS_ASSERT is defined to do something different than the standard assert/NDEBUG. LFS_ASSERT is currently disk-dependent so this is not unreasonable.

To be honest, these unused warnings have been getting a bit more than annoying, and will only get worse if we adopt more compile-time configurations... Is is possible to just disable this warning with -Wno-unused-function?

I'm struggling to come up with an actual programmer mistake this warning protects against...

[bmcdonnell-fb]

@geky thanks for the constructive criticism.

I don't think this works in the case where LFS_ASSERT is defined to do something different than the standard assert/NDEBUG. LFS_ASSERT is currently disk-dependent so this is not unreasonable.

Good point.

Since you want to keep LFS_ASSERT decoupled from NDEBUG, instead of merging this PR, may I suggest you document somewhere that application developers should use their build system / command line options to (conditionally) define LFS_ASSERT? (In my case, that's CMake, and I want to keep the asserts only on debug builds, so I do target_compile_definitions(littlefs PRIVATE $<$<CONFIG:Release>:LFS_NO_ASSERT=1>).)

To be honest, these unused warnings have been getting a bit more than annoying, and will only get worse if we adopt more compile-time configurations... Is is possible to just disable this warning with -Wno-unused-function?

I'm struggling to come up with an actual programmer mistake this warning protects against...

I try to avoid disabling warnings; they are useful. This one helps developers to be sure to remove or use things they may have forgotten about.

[bmcdonnell-fb]

may I suggest you document somewhere that application developers should use their build system / command line options to (conditionally) define LFS_ASSERT?

Or looks like they could put it in their lfs_config.h.

[geky]

application developers should use their build system / command line options to (conditionally) define LFS_ASSERT?

This wouldn't be required if compiling with -Wno-unused-function, no?

I'm thinking a bit more long term. My concern is as littlefs is maturing there is more interest in niche configurations that will chop up the internals more (hypothetical: LFS_READONLY, LFS_2BLOCK, LFS_NO_DIRS, LFS_NO_SEEK, etc). Staying on top of increasingly complicated ifdefs with -Wunused-function may be a waste of time. This code is used and tested, the compiler just doesn't understand how.

Worst case, any unused code will be dropped at link time. -Wno-unused-function doesn't catch unused static inline functions or even non-static functions anyways (-Wmissing-prototypes helps with the latter).

[bmcdonnell-fb]

TL;DR: IMO, disabling warnings is a code smell.

As I said, -Wunused-function "helps developers to be sure to remove or use things they may have forgotten about". That can apply to littlefs, too.

I aim for 0 warnings in anything I'm working on. A build with 0 warnings is easy to check at a glance. Anytime there's a warning, it takes time to spot and interpret. If it's a persistent warning that someone has deemed not a problem, that's repeated developer bandwidth to see, recall, and move on every time. And it makes it easier to miss other warnings.

application developers should use their build system / command line options to (conditionally) define LFS_ASSERT?

This wouldn't be required if compiling with -Wno-unused-function, no?

Correct. You could document that as another option for littlefs users. But please don't only present that option. IMO it should be 2nd choice.

I'm thinking a bit more long term. My concern is as littlefs is maturing there is more interest in niche configurations that will chop up the internals more (hypothetical: LFS_READONLY, LFS_2BLOCK, LFS_NO_DIRS, LFS_NO_SEEK, etc). Staying on top of increasingly complicated ifdefs with -Wunused-function may be a waste of time. This code is used and tested, the compiler just doesn't understand how.

IDK that I have a magic bullet for you, but I hope you and any other contributors will maintain that aspect, and a 0-warning build. The warning catches potential mistakes, and proper #ifdefing shows that developers have considered and handled it.

Worst case, any unused code will be dropped at link time. -Wno-unused-function doesn't catch unused static inline functions or even non-static functions anyways (-Wmissing-prototypes helps with the latter).

Catching some possible mistakes (in this category) is better than catching none of them.

I don't think that's the worst case, though. Like if you forget to use some code that you meant to. Or if application developers disable that warning in their entire application (not just for littlefs), and they forget to use some code. etc.

[geky]

I hope we can agree the worst option is a warning that is neither fixed nor disabled.

If it's a persistent warning that someone has deemed not a problem, that's repeated developer bandwidth to see, recall, and move on every time. And it makes it easier to miss other warnings.

If the warning is counter-productive or unfixable, disabling the warning is warranted. For the same reason you want 0 warnings in the first place. The more useless warnings there are, the less value useful warnings are given.

This is not the first time an issue has been created for an untested configuration warning on 'lfs_mlist_isopen' defined but not used.

I hope you and any other contributors will maintain that aspect, and a 0-warning build.

This is the goal, but we'll have to see how CI keeps up with the number of possible build configurations growing $2^n$...

Catching some possible mistakes (in this category) is better than catching none of them.

Point taken.

Like if you forget to use some code that you meant to.

That's what testing is for. If not calling a vital function goes unnoticed, you probably need more tests. Or you don't actually need to call the function?

Or if application developers disable that warning in their entire application (not just for littlefs), and they forget to use some code. etc.

I'd strongly suggest separate warnings for the application and third-party libraries for the simple reason that you don't control what warnings libraries are developed under. Choosing the lowest-common denominator probably means you're missing out on useful warnings. -Wmissing-prototypes for example does not make sense at the application level.