| Message ID | 920c91eb1e5a1b6d5faa54240dd9c85f72968edc.1744661167.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repack: avoid MIDX'ing cruft pack(s) where possible | expand |
Taylor Blau <me@ttaylorr.com> writes: > @@ -337,6 +337,14 @@ struct object_info { > /* > * Initializer for a "struct object_info" that wants no items. You may > * also memset() the memory to all-zeroes. > + * > + * NOTE: callers expect the initial value of an object_info struct to > + * be zero'd out. Designated initializers like > + * > + * struct object_info oi = { .sizep = &sz }; > + * > + * depend on this behavior, so consider strongly before adding new > + * fields that have a non-zero default value. > */ > #define OBJECT_INFO_INIT { 0 } Hmph, after thinking hard enough, if a developer cannot come up with a way to avoid non-zero default value, the callers could just work if they instead did struct object_info oi = OBJECT_INFO_INIT; oi.sizep = &sz; and the member of non-zero default value can be delat with by updating the default initializer, perhaps like #define OBJECT_INFO_INIT { .enabled = 1 } So I am not sure how the advice in the new comment really helps the intended audiences. Shouldn't the advice be more like NOTE: when a structure foo has FOO_INIT macro to initialize, *never* use your own initialization like so: struct foo foo_instance = { .member_i_care_about = 13 }; Instead, use the _INIT macro and then assign to the member you care about, like so: struct foo foo_instance = FOO_INIT; foo_instance.member_i_care_about = 13; This is because there may be members of "struct foo" whose default value is not zero, or there will later be added such members to the structure. perhaps? I can buy a counter-proposal that does not forbid a custom designated initializers that depends on "all zero" default IF it gives a piece of advice to the readers that is more usable than "consider strongly before adding", as "consider strongly before adding" there smells like just an euphemism for "never add", though. Thanks.
On Mon, Apr 14, 2025 at 1:06 PM Taylor Blau <me@ttaylorr.com> wrote: > > The following commit will use a designated initializer to initialize a > 'struct object_info'. This obviously depends on having the rest of the > fields having a default value of zero, since unspecified fields in a > designated initializer are zero'd out. > > Before writing that designated initializer, I wondered if there were > other spots that also use designated initializers to set up object_info > structs, and there are a handful. > > To prevent potential breakage against future object_info changes that > would introduce/change a field to have a non-zero default value, note > this dependency in a comment near the OBJECT_INFO_INIT macro. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > object-store-ll.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/object-store-ll.h b/object-store-ll.h > index cd3bd5bd99..7ff180d7f2 100644 > --- a/object-store-ll.h > +++ b/object-store-ll.h > @@ -337,6 +337,14 @@ struct object_info { > /* > * Initializer for a "struct object_info" that wants no items. You may > * also memset() the memory to all-zeroes. > + * > + * NOTE: callers expect the initial value of an object_info struct to > + * be zero'd out. Designated initializers like > + * > + * struct object_info oi = { .sizep = &sz }; > + * > + * depend on this behavior, so consider strongly before adding new > + * fields that have a non-zero default value. > */ > #define OBJECT_INFO_INIT { 0 } There are 46 #define'd designated initializers in the code base, from DIR_INIT to OIDMAP_INIT and everything in-between. The logic used in your comment to suggest not using an all-zeroes initializer doesn't seem to depend in any way on something specific to object_info, yet none of those other 46 cases in my quick scanning have such a warning. And 29 of the 46 define some kind of initial value for some fields instead of using all zeroes. That would suggest that one of the following is true: (a) those 29 cases are buggy and shouldn't be doing that, (b) those 29 are all special cases someone has thought through carefully but perhaps someone should add the same warning you have here to those 29 other cases to avoid uncarefully thought cases from being added, (c) there is something specific about object_info that you didn't call out here, or (d) this warning you add is unnecessary.
On Mon, Apr 14, 2025 at 07:57:59PM -0700, Elijah Newren wrote: > There are 46 #define'd designated initializers in the code base, from > DIR_INIT to OIDMAP_INIT and everything in-between. The logic used in > your comment to suggest not using an all-zeroes initializer doesn't > seem to depend in any way on something specific to object_info, yet > none of those other 46 cases in my quick scanning have such a warning. > And 29 of the 46 define some kind of initial value for some fields > instead of using all zeroes. That would suggest that one of the > following is true: (a) those 29 cases are buggy and shouldn't be doing > that, (b) those 29 are all special cases someone has thought through > carefully but perhaps someone should add the same warning you have > here to those 29 other cases to avoid uncarefully thought cases from > being added, (c) there is something specific about object_info that > you didn't call out here, or (d) this warning you add is unnecessary. For (a), I wouldn't say that the _INIT macros are the potentially-buggy component, but rather the use of a designated initializer in the presence of an _INIT macro that initializes the struct to something other than all zeros. I don't think there is anything specific to the object_info structure here, which suggests (d), but I don't think that the warning is unnecessary, it's just overly-specific. I think we should have a convention in CodingGuidelines that forbids designated initializers in structures with non-zero _INIT macros that don't otherwise have a comment about their correctness. But I think there is some discussion to be had there, and I want to disentangle that from this series. I'm going to drop this and the following patch from this series and split them out so we can discuss them more thoroughly while letting this series move ahead. Thanks, Taylor
On Mon, Apr 14, 2025 at 02:07:27PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > @@ -337,6 +337,14 @@ struct object_info { > > /* > > * Initializer for a "struct object_info" that wants no items. You may > > * also memset() the memory to all-zeroes. > > + * > > + * NOTE: callers expect the initial value of an object_info struct to > > + * be zero'd out. Designated initializers like > > + * > > + * struct object_info oi = { .sizep = &sz }; > > + * > > + * depend on this behavior, so consider strongly before adding new > > + * fields that have a non-zero default value. > > */ > > #define OBJECT_INFO_INIT { 0 } > > Hmph, after thinking hard enough, if a developer cannot come up with > a way to avoid non-zero default value, the callers could just work > if they instead did > > struct object_info oi = OBJECT_INFO_INIT; > oi.sizep = &sz; > > and the member of non-zero default value can be delat with by > updating the default initializer, perhaps like > > #define OBJECT_INFO_INIT { .enabled = 1 } Yeah... that's what I was trying to get at with this patch. Basically, "if you have an _INIT macro with non-zero defaults, don't use a custom designated initializer and instead assign fields after using the _INIT macro (which itself is a designated initializer)". But like I wrote to Elijah in the same sub-thread, I think that there is probably more to discuss here, so I ejected this patch from my copy of the series and will re-submit it as its own series later on. Thanks, Taylor
diff --git a/object-store-ll.h b/object-store-ll.h index cd3bd5bd99..7ff180d7f2 100644 --- a/object-store-ll.h +++ b/object-store-ll.h @@ -337,6 +337,14 @@ struct object_info { /* * Initializer for a "struct object_info" that wants no items. You may * also memset() the memory to all-zeroes. + * + * NOTE: callers expect the initial value of an object_info struct to + * be zero'd out. Designated initializers like + * + * struct object_info oi = { .sizep = &sz }; + * + * depend on this behavior, so consider strongly before adding new + * fields that have a non-zero default value. */ #define OBJECT_INFO_INIT { 0 }
The following commit will use a designated initializer to initialize a 'struct object_info'. This obviously depends on having the rest of the fields having a default value of zero, since unspecified fields in a designated initializer are zero'd out. Before writing that designated initializer, I wondered if there were other spots that also use designated initializers to set up object_info structs, and there are a handful. To prevent potential breakage against future object_info changes that would introduce/change a field to have a non-zero default value, note this dependency in a comment near the OBJECT_INFO_INIT macro. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- object-store-ll.h | 8 ++++++++ 1 file changed, 8 insertions(+)