Glen Choo <chooglen@xxxxxxxxxx> writes:
> As someone who isn't that familiar with trailer code, and will have less
> time for the ML soon, this is more of a quick drive-by..
Aren't you also going on vacation soon? ;-)
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> +#define private __private_to_trailer_c__do_not_use
>> +
>> void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
>> {
>> struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
>> strbuf_init(&iter->key, 0);
>> strbuf_init(&iter->val, 0);
>> opts.no_divider = 1;
>> - trailer_info_get(&iter->info, msg, &opts);
>> - iter->cur = 0;
>> + trailer_info_get(&iter->private.info, msg, &opts);
>> + iter->private.cur = 0;
>> }
>> --- a/trailer.h
>> +++ b/trailer.h
>> @@ -119,8 +119,10 @@ struct trailer_iterator {
>> struct strbuf val;
>>...
>> /* private */
>> - struct trailer_info info;
>> - size_t cur;
>> + struct {
>> + struct trailer_info info;
>> + size_t cur;
>> + } __private_to_trailer_c__do_not_use;
>> };
>
> [...]
>
> This is the first instance of this I could find in the codebase. I'm not
> really opposed to having a new way of doing things, but it would be nice
> for us to be consistent with how we handle private members. Other
> approaches I've seen are:
>
> - Using a "larger" struct to hold private members and "downcasting" for
> public users (struct dir_iterator and struct dir_iterator_int). I
> dislike this because I think this enables 'wrong' memory access too
> easily.
> [...]
> - Prefixing private members with "__" (khash.h and other header-only
> libraries use this at least, not sure if we have this in the 'main
> tree'). I think this works pretty well most of the time.
> - Just marking private members with a comment. IMO this is good enough
> the vast majority of the time - if something is private for a good
> reason, it's unlikely to get used accidentally anyway. But properly
> enforcing "privateness" is worthy goal anyway.
Thanks for documenting these other approaches.
I prefer the "larger" struct to hold private members pattern. More
specifically I like the container_of approach pointed out by Jacob [2],
because it is an established pattern in the Linux Kernel and because we
already sort of use the same idea in the list_head type we imported from
the Kernel in 94e99012fc (http-walker: reduce O(n) ops with
doubly-linked list, 2016-07-11). That is, for example for the
new_trailer_item struct we have
struct new_trailer_item {
struct list_head list;
<list item stuff>
};
and to me this is symmetric to the container_of pattern described by Jacob:
struct dir_entry_private {
struct dir_entry entry;
<private stuff>
};
Accordingly, we are already doing the "structure pointer math" (which
Jacob described in [2]) for list_head in list.h:
/* Get typed element from list at a given position. */
#define list_entry(ptr, type, member) \
((type *) ((char *) (ptr) - offsetof(type, member)))
In this patch series though, I decided to just stick with giving the
struct a private-sounding name, because I don't think we reached
consensus on what the preferred approach is for separating
public/private fields.
> (As an aside, if we really wanted to 'strictly' enforce privateness in
> this patch, shouldn't we move the "#define private" into the .c file,
> the way dir_iterator_int is in the .c file?)
I think you meant moving the struct into the .c file (the "#define" is
already in the .c file).
> Personally, I think a decent tradeoff between enforcement and ergonomics
> would be to use an inner struct like you do here, but name it something
> autocomplete-friendly and obviously private, like "private" or
> "_private".
SGTM. I think I'll go with "internal" though, to align with 576de3d956
(unpack_trees: start splitting internal fields from public API,
2023-02-27) which Phillip pointed out. Will reroll.
> I suspect self-regulation and code review should be enough
> to catch nearly all accidental uses of private members.
Ack. In the future, if and when we want compiler-level guarantees to
make it impossible (this was the discussion at [1]), we can revisit this
area.
[1] https://lore.kernel.org/git/16ff5069-0408-21cd-995c-8b47afb9810d@xxxxxxxxxx/
[2] https://lore.kernel.org/git/CA+P7+xo02dGkjb5DwJ1Af_hoQ5HiuxASheZxoFz+r6B-6cQMug@xxxxxxxxxxxxxx/
