Horton

Thread

Commits

  1. Use C99 designated initializers for some structs

  1. Use C99 designated initializers for some structs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-29T10:13:07Z

    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

  2. Re: Use C99 designated initializers for some structs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-29T10:14:03Z

    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

  3. Re: Use C99 designated initializers for some structs

    David Steele <david@pgmasters.net> — 2018-08-29T20:51:07Z

    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

  4. Re: Use C99 designated initializers for some structs

    Andres Freund <andres@anarazel.de> — 2018-08-29T20:56:43Z

    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

  5. Re: Use C99 designated initializers for some structs

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-08-29T21:41:42Z

    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

  6. Re: Use C99 designated initializers for some structs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-29T22:51:24Z

    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

  7. Re: Use C99 designated initializers for some structs

    Chapman Flack <chap@anastigmatix.net> — 2018-08-30T00:35:57Z

    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

  8. Re: Use C99 designated initializers for some structs

    Andres Freund <andres@anarazel.de> — 2018-08-30T04:55:31Z

    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

  9. Re: Use C99 designated initializers for some structs

    Andres Freund <andres@anarazel.de> — 2018-08-30T04:57:36Z

    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

  10. Re: Use C99 designated initializers for some structs

    Mark Dilger <hornschnorter@gmail.com> — 2018-08-30T15:59:05Z

    
> 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

  11. Re: Use C99 designated initializers for some structs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-30T16:54:41Z

    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

  12. Re: Use C99 designated initializers for some structs

    Robert Haas <robertmhaas@gmail.com> — 2018-08-30T19:55:58Z

    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

  13. Re: Use C99 designated initializers for some structs

    Andres Freund <andres@anarazel.de> — 2018-08-30T20:14:31Z

    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

  14. Re: Use C99 designated initializers for some structs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T21:53:25Z

    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

  15. Re: Use C99 designated initializers for some structs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-07T09:45:20Z

    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