Re: [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end
- From
- Linus Arver <linusa@google.com>
- Date
- Sep 14, 2023, 01:21 UTC
- Message-ID
- <owly5y4dlfcg.fsf@fine.c.googlers.com>
- In-Reply-To
- <xmqqmsxsv8ik.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Linus Arver <linusa@google.com> >> >> Previously these fields in the trailer_info struct were of type "const >> char *" and pointed to positions in the input string directly (to the >> start and end positions of the trailer block). >> >> Use offsets to make the intended usage less ambiguous. We only need to >> reference the input string in format_trailer_info(), so update that >> function to take a pointer to the input. > > Hmm, I am not sure if this is an improvement. If the underlying > buffer can be reallocated (to grow), the approach to use the offsets > certainly is easier to deal with, as they will stay valid even after > such a reallocation. But you lose the obvious sentinel value NULL > that can mean something special
True.
Show 21 quoted lines
> and have to make the readers aware > of the local convention you happened to have picked with a comment > like ... > >> Signed-off-by: Linus Arver <linusa@google.com> >> --- >> trailer.c | 17 ++++++++--------- >> trailer.h | 7 +++---- >> 2 files changed, 11 insertions(+), 13 deletions(-) >> ... >> /* >> - * Pointers to the start and end of the trailer block found. If there >> - * is no trailer block found, these 2 pointers point to the end of the >> - * input string. >> + * Offsets to the trailer block start and end positions in the input >> + * string. If no trailer block is found, these are set to 0. >> */ > > ... this, simply because there is no obvious sentinel value for an > unsigned integral type; even if you count MAX_ULONG and its friends, > they are not as obvious as NULL for pointer types.
I agree that there is no trustworthy sentinel value for an unsigned integral type.
On the other hand, we never used NULL as a sentinel value before even when they were const char pointers --- the current comment for these fields which say ...
If there is no trailer block found, these 2 pointers point to the end of the
input string.... sounds somewhat arbitrary to me (and I don't think we care about this property in trailer.c, and AFAICS it's also not something that consumers should be aware of). Consumers of the trailer_info struct could also just see if "info->trailer_nr > 0" to check whether any trailers were found, although if we're merging Patch 1 [1] of this series the consumers will not have easy access any more to any of the trailer_info fields, and they should instead be using a public-facing function that does the "were trailers found" check.
> So, I dunno.
If the "we don't use NULL sentinel values currently anyway" argument is convincing enough, I'm happy to add this to the commit message on a reroll. But I'm also OK with dropping this patch. Thoughts?
[1] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#m8f1b5f1eb346331658c8c7b3e057a4ee31223664