On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers <ndesaulni...@google.com> wrote:
>
> On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o <ty...@mit.edu> wrote:
> >
> > 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
Ok. I will make it clear in next commit messages.
> > >
> > > 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.
>
> I agree.
I agree too.
> >
> > Hence, this is not a warning that we will *ever* be able to enable
> > unconditionally ---
>
> I agree.
>
> > 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
>
> Because not every instance of -Wunreachable-code-aggressive may be that
> pattern.
The goal is to try to detect real bugs. In this instance specifically I
suggested to remove the "if (0) {...}" because it sounded like an
unused code.
If it is useful it is fine to keep.
For now I am only looking for dead code that cannot be enabled
by a configuration file or architecture. In fact, there are several
warnings that I am ignoring because they are a dead code in my
build but may not be in another.
> > 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.
>
> I agree. Hence the examination of instances found by Vinicius.
I liked the idea to create htree_rep_invariant_check. I will be doing
that.
Thanks for the help and suggestions.
> --
> Thanks,
> ~Nick Desaulniers
