On Tue, Jun 11, 2019 at 02:48:51PM -0700, Junio C Hamano wrote: > Matthew DeVore <matvore@xxxxxxxxxxx> writes: > > >> It is brittle because callers are bound to forget doing "if > >> (!x->buf.buf) lazy_init(&x->buf)" at some point, and blindly use an > >> uninitialized x->buf. Making sure x->buf is always initialized > > > > A corallary proposition would be to make this particular strbuf a "struct > > strbuf *" rather than an inline strbuf. It should then be rather clear to users > > that it may be null. > > Would make it less likely for uses of an uninitialized strbuf to be > left undetected as errors? I guess so, and if that is the case it > would definitely be an improvement. > > But initializing the strbuf at the point where the enclosing > structure is initialized (or calloc()'ed) is also a vaiable option, > and between the two, I think that would be even more robust. > > There may be reasons why it is cumbersome to arrange it that way, > though (e.g. if the code does not introduce a "new_stuff()" > allocator that also initializes, and instead uses xcalloc() from > many places, initializing the enclosing structure properly might > take a preliminary clean-up step before the main part of the patch > series can begin). These are all the locations where a struct which ultimately contains a list_objects_filter_options is instantiated: GLOBAL VARIABLES: builtin/clone.c:68:static struct list_objects_filter_options filter_options; builtin/fetch.c:66:static struct list_objects_filter_options filter_options; builtin/pack-objects.c:112:static struct list_objects_filter_options filter_options; builtin/rev-list.c:65:static struct list_objects_filter_options filter_options; LOCAL VARIABLES: builtin/fetch-pack.c:54: struct fetch_pack_args args; transport.c:327: struct fetch_pack_args args; HEAP ALLOCATIONS: transport-helper.c:1123: struct helper_data *data = xcalloc(1, sizeof(*data)); transport.c:964: struct git_transport_data *data = xcalloc(1, sizeof(*data)); git_transport_options is also not directly instantiated as a local or static variable, but it would need to have a git_transport_options_init function defined. I didn't count exactly the number of _INIT macros and _init functions that would need to be defined. It seems like a lot of work. It is hard to believe that our ability to exhaustively pinpoint all these instantiations, and to catch ALL future instantiations, is all that reliable. I think our ability to find the places we need to lazily instantiate the strbuf-containing-struct (struct filter_spec in the interdiff) is more reliable.
