"Linus Arver via GitGitGadget" [off-list ref] writes:
From: Linus Arver <redacted>
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, 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 <redacted>
---
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.
So, I dunno.
Junio C Hamano [off-list ref] writes:
"Linus Arver via GitGitGadget" [off-list ref] writes:
quoted
From: Linus Arver <redacted>
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.
and have to make the readers aware
of the local convention you happened to have picked with a comment
like ...
quoted
Signed-off-by: Linus Arver <redacted>
---
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
Linus Arver [off-list ref] writes:
Junio C Hamano [off-list ref] writes:
quoted
"Linus Arver via GitGitGadget" [off-list ref] writes:
quoted
From: Linus Arver <redacted>
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.
quoted
and have to make the readers aware
of the local convention you happened to have picked with a comment
like ...
quoted
Signed-off-by: Linus Arver <redacted>
---
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.
*/
I've just realized that the new comment "If no trailer block is found,
these are set to 0" is perhaps not always true in the current version
of this patch. This is because this patch did a mechanical type
conversion of the old fields from "const char *" to "size_t", and we
still run
trailer_end = find_trailer_end(str, trailer_search_boundary);
trailer_start = find_trailer_start(str, trailer_end);
inside trailer_info_get() (not visible in the patch context lines). I
will update this patch to make the comment "If no trailer block is found,
these are set to 0" true.