Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

["Theodore Ts'o" <tytso@mit.edu>]

On Mon, Feb 01, 2021 at 07:05:11PM -0300, Vinicius Tinti wrote:
> 
> 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.

The trick was that it was unused code, but it was pretty obviously
deliberate, which should have implied that at some point, it was
considered useful.   :-)

It was the fact that you were so determined to find a way to suppress
the warning, suggesting multiple tactics, which made me wonder.... why
were you going through so much effort to silence the warning if the
goal was *not* to turn it on unconditionally everywhere?

I suspect the much more useful thing to consider is how can we suggest
hueristics to the Clang folks to make the warning more helpful.  For
example, Coverity will warn about the following:

void test_func(unsigned int arg)
{
	if (arg < 0) {
		printf("Hello, world\n")
	}
}

This is an example of dead code that is pretty clearly unintended ---
and it's something that "clang -Wall" or "gcc -Wall" doesn't pick up
on, but Coverity does.

So in cases where the code is explicitly doing "if (0)" or "if
(IS_ENABLED(xxx))" where IS_ENABLED resolves down to zero due to
preprocessor magic, arguably, that's not a useful compiler warning
because it almost *certainly* is intentional.  But in the case of an
unsigned int being compared to see if it is less than, or greater than
or equal to 0, that's almost certainly a bug --- and yes, Coverity has
found a real bug (tm) in my code due to that kind of static code
analysis.  So it would actually be quite nice if there was a compiler
warning (either gcc or clang, I don't really care which) which would
reliably call that out without having the maintainer submit the code
to Coverity for analysis.

Cheers,

							- Ted

P.S.  If anyone wants to file a feature request bug with the Clang
developers, feel free.  :-)