On Tue, Apr 18, 2023 at 12:56 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> It still has magic numbers for the sizes of the fields, should those
> also be named constants?
I thought about that. It's arguable, but personally, I don't think
it's worth it. If the concern is greppability, having constants for
the offsets is good enough for that. If the concern is making it
error-free, I think we'd be well-advised to consider bigger redesigns
of the API. For example, we could have a function
read_number_from_tar_header(char *pointer_to_the_start_of_the_block,
enum which_field) and then that function could encapsulate the
knowledge of which tar numbers are 8 bytes and which are 12 bytes.
Writing read_number_from_tar_header(h, TAR_FIELD_CHECKSUM) seems
potentially less error-prone than
read_tar_number(&h[TAR_OFFSET_CHECKSUM], 8). On the other hand,
changing the latter to read_tar_number(&h[TAR_OFFSET_CHECKSUM],
TAR_LENGTH_CHECKSUM) seems longer but not necessarily cleaner. So I
felt it didn't make sense.
Just to be clear, I don't have a full vision for what a replacement
API ought to look like, and I'm not sure that figuring that out is
something that has to be done right this minute. I proposed this patch
not because it's perfect, but because it's simple. We can think of
doing more in the future if someone wants to devote the effort, and
that person might even be me, but right now it's not.
--
Robert Haas
EDB: http://www.enterprisedb.com
