[PATCH] config.mak.dev: enable -Wunreachable-code
- From
Jeff King <peff@peff.net>
- Date
- Mar 7, 2025, 22:54 UTC
- Message-ID
- <20250307225444.GA42758@coredump.intra.peff.net>
- In-Reply-To
- <xmqq34foefh8.fsf@gitster.g>
On Fri, Mar 07, 2025 at 12:46:27PM -0800, Junio C Hamano wrote:
Show 33 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
> >
> >> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
> >> update->refname,
> >> oid_to_hex(&update->old_oid));
> >> return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
> >> +
> >> + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> >> + strbuf_setlen(err, 0);
> >> + ret = 0;
> >> + continue;
> >> + }
> >> +
> >> goto error;
> >> }
> >> }
> >
> > This new code isn't reachable, since we return in the lines shown in the
> > diff context.
> >
> > Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
> > I think the "goto error" was already unreachable, so possibly the error
> > is in an earlier patch. (I didn't look; Coverity flagged this in the
> > final state in 'jch').
>
> Sorry about that. It shows that I lack the bandwidth necessary to
> go through fine toothed comb on all the topics I queue. Perhaps I
> should be more selective and queue only the ones I personally had
> enough bandwidth to look over (or have seen clear "I looked each and
> every line of this series with fine toothed comb, put reviewed-by:
> me" messages sent by trusted reviewers) while ignoring others?Eh, I would not worry about it too much. Things get missed, and that is why we have many layers of reviews, static analysis, and ultimately users to help us find bugs. ;)
I was disappointed that the compiler didn't complain, though. Maybe we should do this:
-- >8 -- Subject: [PATCH] config.mak.dev: enable -Wunreachable-code
Having the compiler point out unreachable code can help avoid bugs, like the one discussed in:
https://lore.kernel.org/git/20250307195057.GA3675279@coredump.intra.peff.net/
In that case it was found by Coverity, but finding it earlier saves everybody time and effort.
We can use -Wunreachable-code to get some help from the compiler here. Interestingly, this is a noop in gcc. It was a real warning up until gcc 4.x, when it was removed for being too flaky, but they left the command-line option to avoid breaking users. See:
https://stackoverflow.com/questions/17249934/why-does-gcc-not-warn-for-unreachable-code
However, clang does implement this option, and it finds the case mentioned above (and no other cases within the code base). And since we run clang in several of our CI jobs, that's enough to get an early warning of breakage.
We could enable it only for clang, but since gcc is happy to ignore it, it's simpler to just turn it on for all developer builds.
Signed-off-by: Jeff King <peff@peff.net> --- You can see it in action (merged into 'jch') here:
https://github.com/peff/git/actions/runs/13729842188
where all of the clang jobs fail.
config.mak.dev | 1 + 1 file changed, 1 insertion(+)
diff --git a/config.mak.dev b/config.mak.dev index 0fd8cc4d35..95b7bc46ae 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -39,6 +39,7 @@ DEVELOPER_CFLAGS += -Wunused DEVELOPER_CFLAGS += -Wvla DEVELOPER_CFLAGS += -Wwrite-strings DEVELOPER_CFLAGS += -fno-common +DEVELOPER_CFLAGS += -Wunreachable-code ifneq ($(filter clang4,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
-- 2.49.0.rc1.380.g53e738dd21