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
