On Fri, Apr 09 2021, Jeff King wrote:
> On Fri, Apr 09, 2021 at 10:32:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the type_from_string*() functions to return an "enum
>> object_type", but don't refactor their callers to check for "==
>> OBJ_BAD" instead of "< 0".
>>
>> Refactoring the check of the return value to check == OBJ_BAD would
>> now be equivalent to "ret < 0", but the consensus on an earlier
>> version of this patch was to not do that, and to instead use -1
>> consistently as a return value. It just so happens that OBJ_BAD == -1,
>> but let's not put a hard reliance on that.
>
> I think what the patch is doing is good, but this rationale misses the
> main point of that discussion, I think. I doubt that the value of
> OBJ_BAD would ever change. But the point was that we could grow a new
> "failure" value at "-2", and we would want to catch here (I do consider
> it relatively unlikely, but that IMHO is the reason to keep the negative
> check).
>
> I think for the same reason that "return OBJ_BAD" instead of "return -1"
> would be just fine (it is not "just so happens" that OBJ_BAD is
> negative; that was deliberate to allow exactly this convention). But I
> am also OK with leaving the "return -1" calls.
I'm beginning to think in response to this and the comment on 5/6 that
it might be cleaner to split up the object_type enum, as demonstrated
for a config.[ch] feature in [1].
Converting back and forth between them is a bit nasty, and having
multiple interchangable OBJ_* constants with identical values just to
satisfy them being in different enums, but it would allow having the
compiler explicitly help check that callers cover all possible cases of
values they could get.
Most callers just get OBJ_{COMMIT,TREE,BLOB,TAG} some more get that plus
OBJ_{BAD,NONE}, almost nobody gets OBJ_{OFS,REF}_DELTA, and AFAICT just
the peel code cares about OBJ_ANY. We then have an OBJ_MAX nobody's ever
used for anything (I've got some unsubmitted patch somewhere to remove
it).
What do you think about that sort of approach? I haven't convinced
myself that it's a good idea, so far I just thought bridging the gap of
things that return "enum" actually having that as part of their
signature for human legibility, even if C itself doesn't care about the
difference, and we currently can't get much/any of the benefits of the
compiler catching non-exhaustive "case" statements (unless every
callsite is to include OBJ_OFS etc.).
1. https://lore.kernel.org/git/875z0wicmp.fsf@xxxxxxxxxxxxxxxxxxx/
