Re: [PATCH 06/11] t-reftable-record: add ref tests for reftable_record_is_deletion()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:

> On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> Chandra Pratap <chandrapratap3519@xxxxxxxxx> writes:
>> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void)
>> >               switch (i) {
>> >               case REFTABLE_REF_DELETION:
>> > +                     check(reftable_record_is_deletion(&in));
>> >                       break;
>> >               case REFTABLE_REF_VAL1:
>> > +                     check(!reftable_record_is_deletion(&in));
>> >                       set_hash(in.u.ref.value.val1, 1);
>> >                       break;
>>
>> I think it might be easier to follow if we just move this outside the
>> switch, perhaps something like:
>>
>> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void)
>>                 switch (i) {
>>                 case REFTABLE_REF_DELETION:
>> -                       check(reftable_record_is_deletion(&in));
>>                         break;
>>                 case REFTABLE_REF_VAL1:
>> -                       check(!reftable_record_is_deletion(&in));
>>                         set_hash(in.u.ref.value.val1, 1);
>>                         break;
>> @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void)
>> +               check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION);
>
> It's subjective, of course, but for what it's worth, I find Chandra's
> code easier to reason about than this proposed rewrite for at least
> two reasons:
>
> (1) The intention of the original code is obvious at a glance, whereas
> the proposed rewrite requires careful reading and thinking to
> understand what is being tested.
>
> (2) In the original, because the check is being done within each
> `case` arm, it is easy to see how it relates to the case in question,
> whereas in the proposed rewrite, the test is far enough removed from
> from the `switch` that it is more difficult to relate to each possible
> case.

I agree with your statements, my argument was coming more from a point
that the switch statement was used to check and instantiate data into
the structure based on its type.

As such, it would make sense to isolate this away from the checks made
on the same structure.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux