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..
"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;
> };
Interesting approach to "private members". I like that it's fairly
lightweight and clear. On the other hand, I think this will fail to
autocomplete on most people's development setups, and I don't think this
is worth the tradeoff.
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.
(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?)
- 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.
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". I suspect self-regulation and code review should be enough
to catch nearly all accidental uses of private members.
