On Tue, Apr 18, 2023 at 12:56 PM Dagfinn Ilmari Mannsåker <ilm...@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
