[Grep-devel] avoiding new warnings

[Paul Eggert]

- -Wduplicated-branches complained about three false positives,

In Emacs, I found -Wduplicated-branches to be useless and I disabled it. Since
it's finding only false alarms in grep, too, I suggest that we disable it by
default at the Gnulib level. My impression is that for GNU code,
-Wduplicated-branches is mostly just the compiler saying "Look how smart I am!"
rather than something that's helpful in finding real bugs.

[Jim Meyering]

Post by Paul Eggert

- -Wduplicated-branches complained about three false positives,

In Emacs, I found -Wduplicated-branches to be useless and I disabled it.
Since it's finding only false alarms in grep, too, I suggest that we disable
it by default at the Gnulib level. My impression is that for GNU code,
-Wduplicated-branches is mostly just the compiler saying "Look how smart I
am!" rather than something that's helpful in finding real bugs.

Hi Paul,
I'm inclined to wait for more exposure, and/or to let projects disable
it themselves, since two of the three instances it flagged in grep
were odd enough that I feel they benefit from the added attention:

src/kwset.c:759: ptrdiff_t ret = (IGNORE_DUPLICATE_BRANCH_WARNING
src/kwset.c-760- (kwset->trans
src/kwset.c-761- ? bmexec_trans (kwset, text, size)
src/kwset.c-762- : bmexec_trans (kwset, text, size)));
--
src/kwset.c:908: return (IGNORE_DUPLICATE_BRANCH_WARNING
src/kwset.c-909- (kwset->trans
src/kwset.c-910- ? acexec_trans (kwset, text, size, kwsmatch, longest)
src/kwset.c-911- : acexec_trans (kwset, text, size,
kwsmatch, longest)));

In those cases, the duplication is tied to compiler optimization
internals, which makes me think we should have a test to ensure that
this very unusual (and kludgey-feeling) technique is still required.
I.e., it feels like a technique that a compiler optimizer may perform
on its own, rendering this duplication unnecessary.

[Paul Eggert]

Post by Jim Meyering
two of the three instances it flagged in grep
were odd enough that I feel they benefit from the added attention

Anybody reading those two instances of the code would already know what's going
on; it's pretty obvious, it's already commented, and to my mind the extra
attention gets in the way more than it helps.

I agree that it would be better to advise the compiler directly about the
optimization that's needed there, if GCC etc. ever get around to doing that.
(I'm pretty sure the optimization advice is still needed, by the way: static
compilers typically don't have enough information, and even dynamic ones would
have troubles with those two.)

[Jim Meyering]

Post by Paul Eggert

Post by Jim Meyering
two of the three instances it flagged in grep
were odd enough that I feel they benefit from the added attention

Anybody reading those two instances of the code would already know what's
going on; it's pretty obvious, it's already commented, and to my mind the
extra attention gets in the way more than it helps.
I agree that it would be better to advise the compiler directly about the
optimization that's needed there, if GCC etc. ever get around to doing that.
(I'm pretty sure the optimization advice is still needed, by the way: static
compilers typically don't have enough information, and even dynamic ones
would have troubles with those two.)

I feel that the false positives should be sufficiently rare that it's
worth leaving it on by default. It might catch me doing something
stupid even before I run a test :-) Contrast with options like
-Waggregate-return that coreutils, grep, diffutils, gzip and others
disable via configure.ac. If you'd prefer a file-wide directive or
some other syntax in kwset.c, I'll be happy to adjust.

[Paul Eggert]

Post by Jim Meyering
the false positives should be sufficiently rare that it's
worth leaving it on by default. It might catch me doing something
stupid even before I run a test

It might. On the other hand, every warning that -Wduplicated-branches generated
for grep (3 warnings) and for Emacs (4 warnings) was a false alarm. Yes, we can
pacify GCC for each false alarm, but so far -Wduplicated-branches' cost/benefit
ratio has been infinity and this is not a good sign.

I wouldn't bother changing kwset.c back, unless there's a need; we both have
better things to do that worry about this.