Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> @@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>
> get_ref_atom_value(&state, a, s->atom, &va);
> get_ref_atom_value(&state, b, s->atom, &vb);
> - switch (cmp_type) {
> - case FIELD_STR:
> + if (s->version)
> + cmp = versioncmp(va->s, vb->s);
> + else if (cmp_type == FIELD_STR)
> cmp = strcmp(va->s, vb->s);
> - break;
> - default:
> - if (va->ul < vb->ul)
> - cmp = -1;
> - else if (va->ul == vb->ul)
> - cmp = 0;
> - else
> - cmp = 1;
> - break;
> - }
> + else if (va->ul < vb->ul)
> + cmp = -1;
> + else if (va->ul == vb->ul)
> + cmp = 0;
> + else
> + cmp = 1;
> +
So there are generally three kinds of comparison possible:
- if it is to be compared as versions, do versioncmp
- if it is to be compared as strings, do strcmp
- if it is to be compared as numbers, do <=> but because
we are writing in C, not in Perl, do so as if/else/else
Having understood that, the above is not really easy to read and
extend. We should structure the above more like this:
if (s->version)
... versioncmp
else if (... FIELD_STR)
... strcmp
else {
if (a < b)
...
else if (a == b)
...
else
...
}
so that it would be obvious how this code need to be updated
when we need to add yet another kind of comparison.
Without looking at the callers, s->version looks like a misdesign
that should be updated to use the same cmp_type mechanism? That
would lead to even more obvious construct that is easy to enhance,
i.e.
switch (cmp_type) {
case CMP_VERSION:
...
case CMP_STRING:
...
case CMP_NUMBER:
...
}
I dunno.
Other than that (and the structure of that "format-state" stuff we
discussed separately), the series was a pleasant read.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html