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

[bmcdonnell-fb]

No description provided.

[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.

[bmcdonnell-fb]

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

Depends on how you disable it, IMO. If a specific warning is disabled on a line-by-line basis w/ #pragmas, then re-enabled? Yes.

Otherwise... maybe not.

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.

Should be line-by-line, IMO.

...

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?

But isn't that what the warnings are for? It can help you catch things without even any tests, or which may be orthogonal to your tests.

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.

I'm not entirely sure what I think about this in general; I don't recall ever personally disabling a warning at the application level. But I do agree that the lowest common denominator would be a problem.

[bmcdonnell-fb]

Since we've zoomed out to discussing how to deal w/ warnings in general, I'll zoom back in for a moment, to highlight my above suggestion of what to do about this PR:

• Don't merge it
• Document somewhere that application developers should somehow (conditionally) define LFS_ASSERT - whether by their build system / command line options, or in their lfs_config.h.

[geky]

Depends on how you disable it, IMO. If a specific warning is disabled on a line-by-line basis w/ #pragmas, then re-enabled? Yes.

This is not a bad idea. My gut says #pragmas would bring portability issues, but I guess compilers are good at ignoring unknown pragmas... We just may end up with a lot of noise for various compilers. Unfortunately #pragmas are not very macro-able...

I'm not entirely sure what I think about this in general; I don't recall ever personally disabling a warning at the application level. But I do agree that the lowest common denominator would be a problem.

Maybe I just have PTSD from Java's serialVersionUID warnings :)

---

I was probably being too aggressive, but this will be revisited when looking at how littlefs can be chopped up into smaller builds. We'll probably need some robust/repeatable rules/patterns around unused functions to prevent things from getting too messy.

[bmcdonnell-fb]

Depends on how you disable it, IMO. If a specific warning is disabled on a line-by-line basis w/ #pragmas, then re-enabled? Yes.

This is not a bad idea. My gut says #pragmas would bring portability issues, but I guess compilers are good at ignoring unknown pragmas... We just may end up with a lot of noise for various compilers. Unfortunately #pragmas are not very macro-able...

You could maybe #define some macros for portability, something like this.

[geky]

Ahh, I was not aware of _Pragma. That is interesting.