Eric Sunshine [off-list ref] writes:

On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak [off-list ref] wrote:

quoted

Chandra Pratap [off-list ref] writes:

quoted

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.