| Message ID | patch-5.5-7e571667674-20210927T003330Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Designated initializer cleanup & conversion | expand |
Am 27.09.21 um 02:39 schrieb Ævar Arnfjörð Bjarmason: > --- a/cbtree.h > +++ b/cbtree.h > @@ -37,11 +37,12 @@ enum cb_next { > CB_BREAK = 1 > }; > > -#define CBTREE_INIT { .root = NULL } > +#define CBTREE_INIT { 0 } > > static inline void cb_init(struct cb_tree *t) > { > - t->root = NULL; > + struct cb_tree blank = CBTREE_INIT; This could be static const struct cb_tree blank = CBTREE_INIT; > + memcpy(t, &blank, sizeof(*t)); Is *t = blank; not a thing in C? -- Hannes
Hi Ævar On 27/09/2021 01:39, Ævar Arnfjörð Bjarmason wrote: > Use the same pattern for cb_init() as the one established in the > recent refactoring of other such patterns in > 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT > macro, 2021-07-01). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > cbtree.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cbtree.h b/cbtree.h > index a04a312c3f5..dedbb8e2a45 100644 > --- a/cbtree.h > +++ b/cbtree.h > @@ -37,11 +37,12 @@ enum cb_next { > CB_BREAK = 1 > }; > > -#define CBTREE_INIT { .root = NULL } > +#define CBTREE_INIT { 0 } > > static inline void cb_init(struct cb_tree *t) > { > - t->root = NULL; > + struct cb_tree blank = CBTREE_INIT; > + memcpy(t, &blank, sizeof(*t)); > } Slightly off topic but would this be a good site for a compound literal test balloon? *t = (struct cb_tree){ 0 }; Compound literals are in C99 and seem to have been supported by MSVC since 2013 [1]. Best Wishes Phillip [1] https://docs.microsoft.com/en-us/cpp/porting/visual-cpp-what-s-new-2003-through-2015?view=msvc-160#whats-new-for-c-in-visual-studio-2013 > struct cb_node *cb_lookup(struct cb_tree *, const uint8_t *k, size_t klen); >
On Mon, Sep 27 2021, Phillip Wood wrote: > Hi Ævar > > On 27/09/2021 01:39, Ævar Arnfjörð Bjarmason wrote: >> Use the same pattern for cb_init() as the one established in the >> recent refactoring of other such patterns in >> 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT >> macro, 2021-07-01). >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> cbtree.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> diff --git a/cbtree.h b/cbtree.h >> index a04a312c3f5..dedbb8e2a45 100644 >> --- a/cbtree.h >> +++ b/cbtree.h >> @@ -37,11 +37,12 @@ enum cb_next { >> CB_BREAK = 1 >> }; >> -#define CBTREE_INIT { .root = NULL } >> +#define CBTREE_INIT { 0 } >> static inline void cb_init(struct cb_tree *t) >> { >> - t->root = NULL; >> + struct cb_tree blank = CBTREE_INIT; >> + memcpy(t, &blank, sizeof(*t)); >> } > > Slightly off topic but would this be a good site for a compound > literal test balloon? > > *t = (struct cb_tree){ 0 }; > > Compound literals are in C99 and seem to have been supported by MSVC > since 2013 [1]. I think that's a good thing to test out, FWIW I've also tested it on the IBM xlc, Oracle SunCC and HP/UX's aCC, they all seem to accept it. But I'd prefer just doing that in some general follow-up to bd4232fac33 (Merge branch 'ab/struct-init', 2021-07-16), i.e. let's just use the init pattern it established here.
On Mon, Sep 27 2021, Johannes Sixt wrote: > Am 27.09.21 um 02:39 schrieb Ævar Arnfjörð Bjarmason: >> --- a/cbtree.h >> +++ b/cbtree.h >> @@ -37,11 +37,12 @@ enum cb_next { >> CB_BREAK = 1 >> }; >> >> -#define CBTREE_INIT { .root = NULL } >> +#define CBTREE_INIT { 0 } >> >> static inline void cb_init(struct cb_tree *t) >> { >> - t->root = NULL; >> + struct cb_tree blank = CBTREE_INIT; > > This could be > > static const struct cb_tree blank = CBTREE_INIT; *nod*... >> + memcpy(t, &blank, sizeof(*t)); > > Is > *t = blank; > > not a thing in C? > > -- Hannes ...but to both this & the above my reply in the side-thread at https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/ applies. I.e. this is just following a pattern I got from Jeff King & used in bd4232fac33 (Merge branch 'ab/struct-init', 2021-07-16). FWIW with "const" in general I don't use it as much as I'd personally prefer, see e.g. [1] for one recent discussion, but maybe there wouldn't be any push-back in this case... 1. https://lore.kernel.org/git/patch-1.1-c317e6e125e-20210921T124416Z-avarab@gmail.com/
On Mon, Sep 27, 2021 at 01:02:35PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> static inline void cb_init(struct cb_tree *t) > >> { > >> - t->root = NULL; > >> + struct cb_tree blank = CBTREE_INIT; > > > > This could be > > > > static const struct cb_tree blank = CBTREE_INIT; > > *nod*... > [...] > ...but to both this & the above my reply in the side-thread at > https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/ > applies. I.e. this is just following a pattern I got from Jeff King & > used in bd4232fac33 (Merge branch 'ab/struct-init', 2021-07-16). I'm not sure how a compiler would react to the "static const" thing. I tested the compiler output for the "auto" struct case you've written here, and at least gcc and clang are smart enough to just initialize the pointed-to struct directly, with no extra copy. For a "static const" I'm not sure if they'd end up with the same code, or if they'd allocate a struct in the data segment and just memcpy() into place. A non-const static would perhaps push it in the direction of the data/memcpy thing, though the compiler should be well aware that the struct is never changed nor aliased, and thus we're always writing the INIT values. I suspect the performance is not that different either way (the big thing to avoid is initializing an auto struct on the fly and then copying from it, but this is a pretty easy optimization for compilers to get right). > >> + memcpy(t, &blank, sizeof(*t)); > > > > Is > > *t = blank; > > > > not a thing in C? It would be fine to use struct assignment here, and should be equivalent in most compilers. They know about memcpy() and will inline it as appropriate. I think some C programmers tend to prefer memcpy() just because that's how they think. It also wasn't legal in old K&R compilers, but as far as I know was in C89. You have to take care with assignment of flex-structs, of course, but you also have to do so with memcpy(), too. :) > FWIW with "const" in general I don't use it as much as I'd personally > prefer, see e.g. [1] for one recent discussion, but maybe there wouldn't > be any push-back in this case... This isn't a parameter, so I don't think that discussion applies. _If_ you are going to make it a static, I think a const makes sense here (but probably does nothing beyond signaling your intention, because the compiler can see that it is never modified), but I wouldn't bother with either. -Peff
Am 28.09.21 um 01:54 schrieb Jeff King: > On Mon, Sep 27, 2021 at 01:02:35PM +0200, Ævar Arnfjörð Bjarmason wrote: > >>>> static inline void cb_init(struct cb_tree *t) >>>> { >>>> - t->root = NULL; >>>> + struct cb_tree blank = CBTREE_INIT; >>> >>> This could be >>> >>> static const struct cb_tree blank = CBTREE_INIT; >> >> *nod*... >> [...] >> ...but to both this & the above my reply in the side-thread at >> https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/ >> applies. I.e. this is just following a pattern I got from Jeff King & >> used in bd4232fac33 (Merge branch 'ab/struct-init', 2021-07-16). > > I'm not sure how a compiler would react to the "static const" thing. I > tested the compiler output for the "auto" struct case you've written > here, and at least gcc and clang are smart enough to just initialize the > pointed-to struct directly, with no extra copy. Good! Then a deviation from established patterns is not warranted. -- Hannes
Jeff King <peff@peff.net> writes: >> >> + memcpy(t, &blank, sizeof(*t)); >> > >> > Is >> > *t = blank; >> > >> > not a thing in C? > > It would be fine to use struct assignment here, and should be equivalent > in most compilers. They know about memcpy() and will inline it as > appropriate. FWIW, I'd be fine with structure assignment, but we already have too many such memcpy(<ptr>, &<struct>, sizeof(struct)), adding one more is not giving us too much incremental burden for later clean-up. > I think some C programmers tend to prefer memcpy() just because that's > how they think. It also wasn't legal in old K&R compilers, but as far as > I know was in C89. I think so, too.
On Tue, Sep 28 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >>> >> + memcpy(t, &blank, sizeof(*t)); >>> > >>> > Is >>> > *t = blank; >>> > >>> > not a thing in C? >> >> It would be fine to use struct assignment here, and should be equivalent >> in most compilers. They know about memcpy() and will inline it as >> appropriate. > > FWIW, I'd be fine with structure assignment, but we already have too > many such memcpy(<ptr>, &<struct>, sizeof(struct)), adding one more > is not giving us too much incremental burden for later clean-up. > >> I think some C programmers tend to prefer memcpy() just because that's >> how they think. It also wasn't legal in old K&R compilers, but as far as >> I know was in C89. > > I think so, too. Getting back to the topic of this v2 in general, my reading of the discussion since then is that nothing in it necessitated a v3 re-roll to address outstanding issues. If I've got that wrong please shout...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Tue, Sep 28 2021, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >>>> >> + memcpy(t, &blank, sizeof(*t)); >>>> > >>>> > Is >>>> > *t = blank; >>>> > >>>> > not a thing in C? >>> >>> It would be fine to use struct assignment here, and should be equivalent >>> in most compilers. They know about memcpy() and will inline it as >>> appropriate. >> >> FWIW, I'd be fine with structure assignment, but we already have too >> many such memcpy(<ptr>, &<struct>, sizeof(struct)), adding one more >> is not giving us too much incremental burden for later clean-up. >> >>> I think some C programmers tend to prefer memcpy() just because that's >>> how they think. It also wasn't legal in old K&R compilers, but as far as >>> I know was in C89. >> >> I think so, too. > > Getting back to the topic of this v2 in general, my reading of the > discussion since then is that nothing in it necessitated a v3 re-roll to > address outstanding issues. If I've got that wrong please shout... I was hoping that these can hit 'next' soonish.
On 27/09/2021 12:00, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Sep 27 2021, Phillip Wood wrote: > >> Hi Ævar >> >> On 27/09/2021 01:39, Ævar Arnfjörð Bjarmason wrote: >>> Use the same pattern for cb_init() as the one established in the >>> recent refactoring of other such patterns in >>> 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT >>> macro, 2021-07-01). >>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >>> --- >>> cbtree.h | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> diff --git a/cbtree.h b/cbtree.h >>> index a04a312c3f5..dedbb8e2a45 100644 >>> --- a/cbtree.h >>> +++ b/cbtree.h >>> @@ -37,11 +37,12 @@ enum cb_next { >>> CB_BREAK = 1 >>> }; >>> -#define CBTREE_INIT { .root = NULL } >>> +#define CBTREE_INIT { 0 } >>> static inline void cb_init(struct cb_tree *t) >>> { >>> - t->root = NULL; >>> + struct cb_tree blank = CBTREE_INIT; >>> + memcpy(t, &blank, sizeof(*t)); >>> } >> >> Slightly off topic but would this be a good site for a compound >> literal test balloon? >> >> *t = (struct cb_tree){ 0 }; >> >> Compound literals are in C99 and seem to have been supported by MSVC >> since 2013 [1]. > > I think that's a good thing to test out, FWIW I've also tested it on the > IBM xlc, Oracle SunCC and HP/UX's aCC, they all seem to accept it. Thanks for taking the time to test those other systems, it's good to know they support compound literals > But I'd prefer just doing that in some general follow-up to bd4232fac33 > (Merge branch 'ab/struct-init', 2021-07-16), i.e. let's just use the > init pattern it established here. I agree it makes sense to introduce it as a separate series. I'm not sure if there is a pressing need for them but it is the sort of thing that is occasionally useful. Best Wishes Phillip
diff --git a/cbtree.h b/cbtree.h index a04a312c3f5..dedbb8e2a45 100644 --- a/cbtree.h +++ b/cbtree.h @@ -37,11 +37,12 @@ enum cb_next { CB_BREAK = 1 }; -#define CBTREE_INIT { .root = NULL } +#define CBTREE_INIT { 0 } static inline void cb_init(struct cb_tree *t) { - t->root = NULL; + struct cb_tree blank = CBTREE_INIT; + memcpy(t, &blank, sizeof(*t)); } struct cb_node *cb_lookup(struct cb_tree *, const uint8_t *k, size_t klen);
Use the same pattern for cb_init() as the one established in the recent refactoring of other such patterns in 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cbtree.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)