Re: [PATCH] {fetch,upload}-pack: clearly mark unreachable v2 code

From: Stefan Beller <sbeller@google.com>
Date: Mar 30, 2018, 19:06 UTC
Message-ID: <CAGZ79kaS834w1OjGpr9CzLNKcZ50p3jpaMaRxUKWHkvcF6-LoA@mail.gmail.com>
In-Reply-To: <20180330190343.GB171603@google.com>

On Fri, Mar 30, 2018 at 12:03 PM, Brandon Williams <bmwill@google.com> wrote:

Show 18 quoted lines

> On 03/30, Ævar Arnfjörð Bjarmason wrote:
>> Change the switch statement driving upload_pack_v2() and
>> do_fetch_pack_v2() to clearly indicate that the FETCH_DONE case is
>> being handled implicitly by other code, instead of giving the reader
>> the impression that the "continue" statement is needed.
>>
>> This issue was flagged as DEADCODE by Coverity[1]. Simply removing the
>> "case FETCH_DONE" would make -Wswitch warn. Instead implement the same
>> solution discussed for my "[PATCH v2 18/29] grep: catch a missing enum
>> in switch statement" patch[2] (which never made it into git.git).
>>
>> 1. https://public-inbox.org/git/CAGZ79kbAOcwaRzjuMtZ_HVsYvUr_7UAPbOcnrmPgsdE19q=PrQ@mail.gmail.com/
>> 2. https://public-inbox.org/git/20170513231509.7834-19-avarab@gmail.com/
>
> I understand why you want this change, but I dislike it because it
> removes the ability to have the compiler tell you that your switch
> statements are exhaustive.  Of course it should be noticed rather
> quickly by the addition of those BUG statements :)

I think coverity doesn't flag empty sections, i.e.

    case FETCH_DONE:
    default:
        BUG(...)

would do for coverity. Not sure if we want to add a /*fall thru */ comment, that would aid other compilers to not warn about it.

This would cover both the compiler as well as coverity.

Stefan