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.
