On 07/08/2023 22:16, Glen Choo wrote:
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..
This is a bit of a drive-by comment as well ...
"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.
We have something similar in unpack_trees.h see 576de3d9560
(unpack_trees: start splitting internal fields from public API,
2023-02-27). That adds an "internal" member to "sturct unpack_trees" of
type "struct unpack_trees_internal which seems to be a easier naming scheme.
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?)
That #define is pretty ugly
Another common scheme is to have an opaque pointer to the private struct
in the public struct (aka pimpl idiom). The merge machinery uses this
- see merge-recursive.h. (I'm working on something similar for the
sequencer so we can change the internals without having to re-compile
everything that includes "sequencer.h")
- 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.
It is common but I think the C standard reserves names beginning with "__"
- 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 agree, something like that would match the unpack_trees example
I suspect self-regulation and code review should be enough
to catch nearly all accidental uses of private members.
Agreed
Best Wishes
Phillip
