On Mon, Feb 1, 2021 at 1:09 PM Theodore Ts'o <[email protected]> wrote:
>
> On Mon, Feb 01, 2021 at 03:41:50PM -0300, Vinicius Tinti wrote:
> >
> > My goal is to avoid having a dead code. Three options come to mind.
> >
> > The first would be to add another #ifdef SOMETHING (suggest a name).
> > But this doesn't remove the code and someone could enable it by accident.
>
> I *really* don't see the point of having the compiler whine about
> "dead code", so I'm not terribly fond of
> -Wunreachable-code-aggressive.
I agree; Vinicius, my recommendation for -Wunreachable-* with Clang
was to see whether dead code identified by this more aggressive
diagnostic (than -Wunused-function) was to ask maintainers whether
code identified by it was intentionally dead and if they would
consider removing it. If they say "no," that's fine, and doesn't need
to be pushed. It's not clear to maintainers that:
1. this warning is not on by default
2. we're not looking to pursue turning this on by default
If maintainers want to keep the dead code, that's fine, let them and
move on to the next instance to see if that's interesting (or not).
--
Thanks,
~Nick Desaulniers
On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote:
> I agree; Vinicius, my recommendation for -Wunreachable-* with Clang
> was to see whether dead code identified by this more aggressive
> diagnostic (than -Wunused-function) was to ask maintainers whether
> code identified by it was intentionally dead and if they would
> consider removing it. If they say "no," that's fine, and doesn't need
> to be pushed. It's not clear to maintainers that:
> 1. this warning is not on by default
> 2. we're not looking to pursue turning this on by default
>
> If maintainers want to keep the dead code, that's fine, let them and
> move on to the next instance to see if that's interesting (or not).
It should be noted that in Documenting/process/coding-style.rst, there
is an expicit recommendation to code in a way that will result in dead
code warnings:
Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
symbol into a C boolean expression, and use it in a normal C conditional:
.. code-block:: c
if (IS_ENABLED(CONFIG_SOMETHING)) {
...
}
The compiler will constant-fold the conditional away, and include or exclude
the block of code just as with an #ifdef, so this will not add any runtime
overhead. However, this approach still allows the C compiler to see the code
inside the block, and check it for correctness (syntax, types, symbol
references, etc). Thus, you still have to use an #ifdef if the code inside the
block references symbols that will not exist if the condition is not met.
So our process documentation *explicitly* recommends against using
#ifdef CONFIG_XXX ... #endif, and instead use something that will
-Wunreachable-code-aggressive to cause the compiler to complain.
Hence, this is not a warning that we will *ever* be able to enable
unconditionally --- so why work hard to remove such warnings from the
code? If the goal is to see if we can detect real bugs using this
technique, well and good. If the data shows that this warning
actually is useful in finding bugs, then manybe we can figure out a
way that we can explicitly hint to the compiler that in *this* case,
the maintainer actually knew what they were doing.
But if an examination of the warnings shows that
-Wunreachable-code-aggressive isn't actually finding any real bugs,
then perhaps it's not worth it.
Cheers,
- Ted