Обсуждение: Use C99 designated initializers for some structs
Here is a patch to change some struct initializations to use C99-style designated initializers. These are just a few particularly egregious cases that were hard to read and write, and error prone because of many similar adjacent types. (The PL/Python changes currently don't compile with Python 3 because of the situation described in the parallel thread "PL/Python: Remove use of simple slicing API".) Thoughts? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 29/08/2018 12:13, Peter Eisentraut wrote: > Here is a patch to change some struct initializations to use C99-style > designated initializers. These are just a few particularly egregious > cases that were hard to read and write, and error prone because of many > similar adjacent types. > > (The PL/Python changes currently don't compile with Python 3 because of > the situation described in the parallel thread "PL/Python: Remove use of > simple slicing API".) > > Thoughts? and with the actual patch -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 8/29/18 5:14 AM, Peter Eisentraut wrote: > On 29/08/2018 12:13, Peter Eisentraut wrote: >> Here is a patch to change some struct initializations to use C99-style >> designated initializers. These are just a few particularly egregious >> cases that were hard to read and write, and error prone because of many >> similar adjacent types. >> >> (The PL/Python changes currently don't compile with Python 3 because of >> the situation described in the parallel thread "PL/Python: Remove use of >> simple slicing API".) >> >> Thoughts? +1. This is an incredible win for readability/maintainability. One thing: I'm not sure that excluding the InvalidOid assignment in the TopTransactionStateData initializer is a good idea. That is, it's not clear that InvalidOid is 0. NULL, false, and 0 seem like no-brainers, but maybe it would be better to explicitly include constants that we define that are not obviously 0, or maybe just all of them. Regards, -- -David david@pgmasters.net
Hi, On 2018-08-29 15:51:07 -0500, David Steele wrote: > One thing: I'm not sure that excluding the InvalidOid assignment in the > TopTransactionStateData initializer is a good idea. That is, it's not clear > that InvalidOid is 0. > > NULL, false, and 0 seem like no-brainers, but maybe it would be better to > explicitly include constants that we define that are not obviously 0, or > maybe just all of them. We rely on that in many other places imo. So I don't think it's worth starting to care about it here. Greetings, Andres Freund
On Thu, Aug 30, 2018 at 8:51 AM David Steele <david@pgmasters.net> wrote: > On 8/29/18 5:14 AM, Peter Eisentraut wrote: > > On 29/08/2018 12:13, Peter Eisentraut wrote: > >> Here is a patch to change some struct initializations to use C99-style > >> designated initializers. These are just a few particularly egregious > >> cases that were hard to read and write, and error prone because of many > >> similar adjacent types. > >> > >> (The PL/Python changes currently don't compile with Python 3 because of > >> the situation described in the parallel thread "PL/Python: Remove use of > >> simple slicing API".) > >> > >> Thoughts? > > +1. This is an incredible win for readability/maintainability. +1. Much nicer. I know several people have the goal of being able to compile PostgreSQL as C and C++, and this syntax is not in the C++ standard. Happily, popular compilers seem OK with, and it's already been voted into the draft for C++20. So no problem on that front. -- Thomas Munro http://www.enterprisedb.com
Andres Freund <andres@anarazel.de> writes:
> On 2018-08-29 15:51:07 -0500, David Steele wrote:
>> One thing: I'm not sure that excluding the InvalidOid assignment in the
>> TopTransactionStateData initializer is a good idea. That is, it's not clear
>> that InvalidOid is 0.
>> NULL, false, and 0 seem like no-brainers, but maybe it would be better to
>> explicitly include constants that we define that are not obviously 0, or
>> maybe just all of them.
> We rely on that in many other places imo. So I don't think it's worth
> starting to care about it here.
I agree that assuming that they're physically zeroes is OK from a
portability standpoint, because we'd have a whole lot of other issues
if they weren't. But I have a different point to make, which is that
it's fairly standard practice for us to initialize all fields of a struct
explicitly, even when setting them to zero/false/NULL. I don't think we
should walk away from that practice just because C99 offers a shiny new
syntax for doing so.
The main practical advantage I see to writing such "redundant" explicit
field initializations is that it aids greppability: when you're adding a
new field Y beside field X, grepping for X is a good way of finding the
places where you need to initialize/copy/write/read/generically-frob Y
too. Omitting mention of X just because you're implicitly initializing
it puts a big hole in the reliability of that technique.
As against that, of course, explicitly zeroing fields that you know very
well are already zero eats some cycles. I've occasionally wondered if
it's worth doing things like
mynode = makeNode(MyNodeType);
mynode->w = 42;
/* mynode->x = 0; */
/* mynode->y = NULL; */
mynode->z = ptr;
but that seems mighty ugly.
An argument I *don't* buy is that leaving out redundant field assignments
is a good savings of programmer time. It's not a useful savings to the
original developer, and it's a net negative for anyone who has to review
or modify such code later. I think it was Brooks who said something to
the effect that any programmer would happily type out every line of code
thrice over, if only that would guarantee no bugs. It doesn't, of course,
but there are virtues in verbosity nonetheless.
regards, tom lane
On 08/29/18 18:51, Tom Lane wrote: > As against that, of course, explicitly zeroing fields that you know very > well are already zero eats some cycles. I've occasionally wondered if I haven't checked what a smart C99 compiler actually emits for a designated initializer giving a field a compile-time known constant zero. Is it sure to eat any more cycles than the same initializer with the field unmentioned? -Chap
Hi, On 2018-08-29 20:35:57 -0400, Chapman Flack wrote: > On 08/29/18 18:51, Tom Lane wrote: > > > As against that, of course, explicitly zeroing fields that you know very > > well are already zero eats some cycles. I've occasionally wondered if > > I haven't checked what a smart C99 compiler actually emits for a > designated initializer giving a field a compile-time known constant zero. > Is it sure to eat any more cycles than the same initializer with the field > unmentioned? It's unlikely that any compiler worth its salt will emit redundant zero initializations after a struct initialization (it's dead trivial to recognize than in any SSA like form, which I think most compilers use these days, certainly gcc and clang). What it can't optimize away however is the x = makeNode(SomeType); x->foo = EquivalentToZero; case. Currently the compiler has no way to know that the memory is zero initialized (except for the type member, which the compiler can see being set). Greetings, Andres Freund
Hi, On 2018-08-29 18:51:24 -0400, Tom Lane wrote: > I agree that assuming that they're physically zeroes is OK from a > portability standpoint, because we'd have a whole lot of other issues > if they weren't. But I have a different point to make, which is that > it's fairly standard practice for us to initialize all fields of a struct > explicitly, even when setting them to zero/false/NULL. I don't think we > should walk away from that practice just because C99 offers a shiny new > syntax for doing so. > > The main practical advantage I see to writing such "redundant" explicit > field initializations is that it aids greppability: when you're adding a > new field Y beside field X, grepping for X is a good way of finding the > places where you need to initialize/copy/write/read/generically-frob Y > too. Omitting mention of X just because you're implicitly initializing > it puts a big hole in the reliability of that technique. FWIW, I think this has for bigger costs than gains. You can't rely on it being done everywhere anyway - there's *heaps* of places were we don't set all members - and it makes changing fieldnames etc. way more verbose than it has to be. Greetings, Andres Freund
> On Aug 29, 2018, at 1:51 PM, David Steele <david@pgmasters.net> wrote:
>
> On 8/29/18 5:14 AM, Peter Eisentraut wrote:
>> On 29/08/2018 12:13, Peter Eisentraut wrote:
>>> Here is a patch to change some struct initializations to use C99-style
>>> designated initializers. These are just a few particularly egregious
>>> cases that were hard to read and write, and error prone because of many
>>> similar adjacent types.
>>>
>>> (The PL/Python changes currently don't compile with Python 3 because of
>>> the situation described in the parallel thread "PL/Python: Remove use of
>>> simple slicing API".)
>>>
>>> Thoughts?
>
> +1. This is an incredible win for readability/maintainability.
I tried doing this perhaps a year ago, and there are a few files with arrays
of structs whose representations get much larger when you change the format
in this way. For instance, in guc.c:
static struct config_bool ConfigureNamesBool[] =
{
{
{"enable_seqscan", PGC_USERSET, QUERY_TUNING_METHOD,
gettext_noop("Enables the planner's use of sequential-scan plans."),
NULL
},
&enable_seqscan,
true,
NULL, NULL, NULL
},
becomes:
static struct config_bool ConfigureNamesBool[] =
{
{
.gen = {
.name = "enable_seqscan",
.context = PGC_USERSET,
.group = QUERY_TUNING_METHOD,
.short_desc = gettext_noop("Enables the planner's use of sequential-scan plans."),
.long_desc = NULL
},
.variable = &enable_seqscan,
.boot_val = true,
.check_hook = NULL,
.assign_hook = NULL,
.show_hook = NULL
},
and then gets much longer if you do as per Tom's general suggestion and make
each field explicit (though Tom might not apply that rule to this case):
static struct config_bool ConfigureNamesBool[] =
{
{
.gen = {
.name = "enable_seqscan",
.context = PGC_USERSET,
.group = QUERY_TUNING_METHOD,
.short_desc = gettext_noop("Enables the planner's use of sequential-scan plans."),
.long_desc = NULL,
.flags = 0,
.vartype = 0,
.status = 0,
.source = 0,
.reset_source = 0,
.scontext = 0,
.reset_scontext = 0,
.stack = NULL,
.extra = NULL,
.sourcefile = NULL,
.sourceline = 0
},
.variable = &enable_seqscan,
.boot_val = true,
.check_hook = NULL,
.assign_hook = NULL,
.show_hook = NULL,
.reset_val = false,
.reset_extra = NULL
},
This sort of thing happens an awful lot in guc.c, but it comes up in syscache.c
also, and perhaps other places, though I don't recall any longer which other files
were like this.
What should the general rule be for initializing arrays of structs such as these?
mark
On 2018-Aug-30, Mark Dilger wrote:
> static struct config_bool ConfigureNamesBool[] =
> {
> {
> {"enable_seqscan", PGC_USERSET, QUERY_TUNING_METHOD,
> gettext_noop("Enables the planner's use of sequential-scan plans."),
> NULL
> },
> &enable_seqscan,
> true,
> NULL, NULL, NULL
> },
Personally, I dislike this form -- it's very opaque and I have to refer
to the struct definition each time I want to add a new member, to make
sure I'm assigning the right thing. I welcome designated initializers
in this case even though it becomes more verbose. I don't think
explicitly initializing to NULLs is sensible in this case; let's just
omit those fields.
> What should the general rule be for initializing arrays of structs such as these?
I don't know what a general rule would be. Maybe we can try hand-
inspecting a few cases, and come up with a general rule once we acquire
sufficient experience.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 29, 2018 at 6:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I agree that assuming that they're physically zeroes is OK from a > portability standpoint, because we'd have a whole lot of other issues > if they weren't. But I have a different point to make, which is that > it's fairly standard practice for us to initialize all fields of a struct > explicitly, even when setting them to zero/false/NULL. I don't think we > should walk away from that practice just because C99 offers a shiny new > syntax for doing so. I hate that style with a fiery passion that cannot be quenched. I think you're basically the only one who has routinely done it like this, and so it results in uselessly wasting a lot of mental energy trying to decide whether a new member ought to be handled like the ones Tom added or the ones somebody else added. It makes initializers that could be quite short and compact long and hard to read. In InitProcess(), it's entirely unclear which of those initializations are merely insurance and which ones are actually doing something useful, and there and in other places it's quite hard to tell whether we might be wasting cycles (or instruction cache space) in sufficient quantities to care about. If it were up to me, which it isn't, we'd remove every useless initialize-to-zero statement in the entire code base and then have a party to celebrate their demise. The party would include burning the removed code in effigy. :-) All that being said, this is MOSTLY a coding style issue and it comes down to a matter of preference, so in my opinion, it doesn't really matter that much in the end what we decide to do. Still, my preference would definitely be for nuking it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi,
On 2018-08-30 13:54:41 -0300, Alvaro Herrera wrote:
> On 2018-Aug-30, Mark Dilger wrote:
>
> > static struct config_bool ConfigureNamesBool[] =
> > {
> > {
> > {"enable_seqscan", PGC_USERSET, QUERY_TUNING_METHOD,
> > gettext_noop("Enables the planner's use of sequential-scan plans."),
> > NULL
> > },
> > &enable_seqscan,
> > true,
> > NULL, NULL, NULL
> > },
>
> Personally, I dislike this form -- it's very opaque and I have to refer
> to the struct definition each time I want to add a new member, to make
> sure I'm assigning the right thing.
Dito. Especially because it looks different for the different types of
GUCs.
> I welcome designated initializers in this case even though it becomes
> more verbose. I don't think explicitly initializing to NULLs is
> sensible in this case; let's just omit those fields.
Yea - I mean a large portion of them previously weren't initialized
either, so there's really not a good reason to change that now.
> > What should the general rule be for initializing arrays of structs such as these?
>
> I don't know what a general rule would be. Maybe we can try hand-
> inspecting a few cases, and come up with a general rule once we acquire
> sufficient experience.
I think we should have as rules:
1) Members should be defined in the same order as in the struct, that's
the requirement C++ standard is going to impose. Think it's also
reasonable stylistically.
2) It's OK to omit setting members if zero-initialization obviously is
correct.
We probably should also check how well pgindent copes, and whether that
dictates some formatting choices.
Greetings,
Andres Freund
Mark Dilger <hornschnorter@gmail.com> writes:
> I tried doing this perhaps a year ago, and there are a few files with arrays
> of structs whose representations get much larger when you change the format
> in this way. For instance, in guc.c:
> ...
> What should the general rule be for initializing arrays of structs such as these?
I'd argue that there is no reason to convert initializers except where
it's a clear notational win. If it isn't, leaving things alone will
be less of a maintenance problem.
regards, tom lane
On 30/08/2018 22:14, Andres Freund wrote: > I think we should have as rules: > > 1) Members should be defined in the same order as in the struct, that's > the requirement C++ standard is going to impose. Think it's also > reasonable stylistically. > 2) It's OK to omit setting members if zero-initialization obviously is > correct. It seems like most people were OK with that, so I committed the patch. This is something that we'll likely gain more experience with over time. > We probably should also check how well pgindent copes, and whether that > dictates some formatting choices. The patch I submitted was run through pgindent. I did not experience any problem, and it didn't reformat anything about what I had originally typed in (except one comment I think). -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services