Thread: Proposal: Make use of C99 designated initialisers for nulls/valuesarrays
Dear Hackers, I have identified some OSS code which maybe can make use of C99 designated initialisers for nulls/values arrays. ~ Background: There are lots of tuple operations where arrays of values and flags are being passed. Typically these arrays are being previously initialised 0/false by memset. By modifying code to use C99 designated initialiser syntax [1], most of these memsets can become redundant. Actually, this mechanism is already being used in some of the existing OSS code. This patch/proposal just propagates thesame idea to all other similar places I could find. ~ Result: Less code. Removes ~200 unnecessary memsets. More consistent initialisation. ~ Typical Example: Before: Datum values[Natts_pg_attribute]; bool nulls[Natts_pg_attribute]; ... memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); After: Datum values[Natts_pg_attribute] = {0}; bool nulls[Natts_pg_attribute] = {0}; --- [1] REF C99 [$6.7.8/21] If there are fewer initializers in a brace-enclosed list than there are elements or members of anaggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration ~ Please refer to the attached patch. Kind Regards, --- Peter Smith Fujitsu Australia
Attachment
On Tue, Oct 1, 2019 at 1:25 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote: > > Dear Hackers, > > I have identified some OSS code which maybe can make use of C99 designated initialisers for nulls/values arrays. > > ~ > > Background: > There are lots of tuple operations where arrays of values and flags are being passed. > Typically these arrays are being previously initialised 0/false by memset. > By modifying code to use C99 designated initialiser syntax [1], most of these memsets can become redundant. > Actually, this mechanism is already being used in some of the existing OSS code. This patch/proposal just propagates thesame idea to all other similar places I could find. > > ~ > > Result: > Less code. Removes ~200 unnecessary memsets. > More consistent initialisation. > +1. This seems like an improvement. I can review and take this forward unless there are objections from others. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andrew Dunstan
Date:
On 10/1/19 6:12 AM, Amit Kapila wrote: > On Tue, Oct 1, 2019 at 1:25 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote: >> Dear Hackers, >> >> I have identified some OSS code which maybe can make use of C99 designated initialisers for nulls/values arrays. >> >> ~ >> >> Background: >> There are lots of tuple operations where arrays of values and flags are being passed. >> Typically these arrays are being previously initialised 0/false by memset. >> By modifying code to use C99 designated initialiser syntax [1], most of these memsets can become redundant. >> Actually, this mechanism is already being used in some of the existing OSS code. This patch/proposal just propagates thesame idea to all other similar places I could find. >> >> ~ >> >> Result: >> Less code. Removes ~200 unnecessary memsets. >> More consistent initialisation. >> > +1. This seems like an improvement. I can review and take this > forward unless there are objections from others. > > +1. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Isaac Morland
Date:
On Tue, 1 Oct 2019 at 03:55, Smith, Peter <peters@fast.au.fujitsu.com> wrote:
Typical Example:
Before:
Datum values[Natts_pg_attribute];
bool nulls[Natts_pg_attribute];
...
memset(values, 0, sizeof(values));
memset(nulls, false, sizeof(nulls));
After:
Datum values[Natts_pg_attribute] = {0};
bool nulls[Natts_pg_attribute] = {0};
I hope you'll forgive a noob question. Why does the "After" initialization for the boolean array have {0} rather than {false}?
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Bruce Momjian
Date:
On Tue, Oct 1, 2019 at 08:40:26AM -0400, Andrew Dunstan wrote: > > On 10/1/19 6:12 AM, Amit Kapila wrote: > > On Tue, Oct 1, 2019 at 1:25 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote: > >> Dear Hackers, > >> > >> I have identified some OSS code which maybe can make use of C99 designated initialisers for nulls/values arrays. > >> > >> ~ > >> > >> Background: > >> There are lots of tuple operations where arrays of values and flags are being passed. > >> Typically these arrays are being previously initialised 0/false by memset. > >> By modifying code to use C99 designated initialiser syntax [1], most of these memsets can become redundant. > >> Actually, this mechanism is already being used in some of the existing OSS code. This patch/proposal just propagatesthe same idea to all other similar places I could find. > >> > >> ~ > >> > >> Result: > >> Less code. Removes ~200 unnecessary memsets. > >> More consistent initialisation. > >> > > +1. This seems like an improvement. I can review and take this > > forward unless there are objections from others. > > > > > > +1. I like it! -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Bruce Momjian <bruce@momjian.us> writes: >>> On Tue, Oct 1, 2019 at 1:25 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote: >>>> There are lots of tuple operations where arrays of values and flags are being passed. >>>> Typically these arrays are being previously initialised 0/false by memset. >>>> By modifying code to use C99 designated initialiser syntax [1], most of these memsets can become redundant. > I like it! FYI, I checked into whether this would result in worse generated code. In the one place I checked (InsertPgAttributeTuple, which hopefully is representative), I got *exactly the same* assembly code before and after, on both a somewhat-aging gcc and fairly modern clang. Hadn't quite expected that, but it removes any worries about whether we might be losing anything. Note though that InsertPgAttributeTuple uses memset(), while some of these other places use MemSet(). The code I see being generated for MemSet() is also the same(!) on clang, but it is different and probably worse on gcc. I wonder if it isn't time to kick MemSet to the curb. We have not re-evaluated that macro in more than a dozen years, and compilers have surely changed. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
Hi, On 2019-10-01 12:17:08 -0400, Tom Lane wrote: > FYI, I checked into whether this would result in worse generated code. > In the one place I checked (InsertPgAttributeTuple, which hopefully > is representative), I got *exactly the same* assembly code before > and after, on both a somewhat-aging gcc and fairly modern clang. > Hadn't quite expected that, but it removes any worries about whether > we might be losing anything. I think the only case where it's plausible to be really worse is where we intentionally leave part of such allocations uninitialized - which we can't easily do in these cases because the rest of the struct will also get zeroed out. The compiler will probably figure it out in some cases, but there's plenty where it can't. But I don't think there's many places like that in our code though. > Note though that InsertPgAttributeTuple uses memset(), while some of > these other places use MemSet(). The code I see being generated for > MemSet() is also the same(!) on clang, but it is different and > probably worse on gcc. I wonder if it isn't time to kick MemSet to > the curb. We have not re-evaluated that macro in more than a dozen > years, and compilers have surely changed. Yes, we really should! - Andres
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Thomas Munro
Date:
On Wed, Oct 2, 2019 at 5:49 AM Andres Freund <andres@anarazel.de> wrote: > On 2019-10-01 12:17:08 -0400, Tom Lane wrote: > > Note though that InsertPgAttributeTuple uses memset(), while some of > > these other places use MemSet(). The code I see being generated for > > MemSet() is also the same(!) on clang, but it is different and > > probably worse on gcc. I wonder if it isn't time to kick MemSet to > > the curb. We have not re-evaluated that macro in more than a dozen > > years, and compilers have surely changed. > > Yes, we really should! +1 FWIW I experimented with that over here: https://www.postgresql.org/message-id/CA%2BhUKGLfa6ANa0vs7Lf0op0XBH05HE8SyX8NFhDyT7k2CHYLXw%40mail.gmail.com
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Isaac Morland <isaac.morland@gmail.com> Sent: Tuesday, 1 October 2019 11:32 PM >Typical Example: >Before: > Datum values[Natts_pg_attribute]; > bool nulls[Natts_pg_attribute]; > ... > memset(values, 0, sizeof(values)); > memset(nulls, false, sizeof(nulls)); >After: > Datum values[Natts_pg_attribute] = {0}; > bool nulls[Natts_pg_attribute] = {0}; > >I hope you'll forgive a noob question. Why does the "After" initialization for the boolean array have {0} rather than {false}? It is a valid question. I found that the original memsets that this patch replaces were already using 0 and false interchangeably. So I just pickedone. Reasons I chose {0} over {false} are: (a) laziness, and (b) consistency with the values[] initialiser. But it is no problem to change the bool initialisers to {false} if that becomes a committer review issue. Kind Regards -- Peter Smith Fujitsu Australia
On Wed, Oct 2, 2019 at 4:53 AM Smith, Peter <peters@fast.au.fujitsu.com> wrote:
From: Isaac Morland <isaac.morland@gmail.com> Sent: Tuesday, 1 October 2019 11:32 PM
>Typical Example:
>Before:
> Datum values[Natts_pg_attribute];
> bool nulls[Natts_pg_attribute];
> ...
> memset(values, 0, sizeof(values));
> memset(nulls, false, sizeof(nulls));
>After:
> Datum values[Natts_pg_attribute] = {0};
> bool nulls[Natts_pg_attribute] = {0};
>
>I hope you'll forgive a noob question. Why does the "After" initialization for the boolean array have {0} rather than {false}?
It is a valid question.
I found that the original memsets that this patch replaces were already using 0 and false interchangeably. So I just picked one.
Reasons I chose {0} over {false} are: (a) laziness, and (b) consistency with the values[] initialiser.
In this case, I think it is better to be consistent in all the places. As of now (without patch), we are using 'false' or '0' to initialize the boolean array. See below two instances from the patch:
1.
@@ -607,9 +601,9 @@ UpdateStatisticsForTypeChange(Oid statsOid, Oid relationOid, int attnum,
Relation rel;
- Datum values[Natts_pg_statistic_ext_data];
- bool nulls[Natts_pg_statistic_ext_data];
- bool replaces[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext_data] = {0};
+ bool nulls[Natts_pg_statistic_ext_data] = {0};
+ bool replaces[Natts_pg_statistic_ext_data] = {0};
oldtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid));
if (!HeapTupleIsValid(oldtup))
@@ -630,10 +624,6 @@ UpdateStatisticsForTypeChange(Oid statsOid, Oid relationOid, int attnum,
* OK, we need to reset some statistics. So let's build the new tuple,
* replacing the affected statistics types with NULL.
*/
- memset(nulls, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(replaces, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(values, 0, Natts_pg_statistic_ext_data * sizeof(Datum));
Relation rel;
- Datum values[Natts_pg_statistic_ext_data];
- bool nulls[Natts_pg_statistic_ext_data];
- bool replaces[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext_data] = {0};
+ bool nulls[Natts_pg_statistic_ext_data] = {0};
+ bool replaces[Natts_pg_statistic_ext_data] = {0};
oldtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid));
if (!HeapTupleIsValid(oldtup))
@@ -630,10 +624,6 @@ UpdateStatisticsForTypeChange(Oid statsOid, Oid relationOid, int attnum,
* OK, we need to reset some statistics. So let's build the new tuple,
* replacing the affected statistics types with NULL.
*/
- memset(nulls, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(replaces, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(values, 0, Natts_pg_statistic_ext_data * sizeof(Datum));
2.
@@ -69,10 +69,10 @@ CreateStatistics(CreateStatsStmt *stmt)
Oid namespaceId;
Oid stxowner = GetUserId();
HeapTuple htup;
- Datum values[Natts_pg_statistic_ext];
- bool nulls[Natts_pg_statistic_ext];
- Datum datavalues[Natts_pg_statistic_ext_data];
- bool datanulls[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext] = {0};
+ bool nulls[Natts_pg_statistic_ext] = {0};
+ Datum datavalues[Natts_pg_statistic_ext_data] = {0};
+ bool datanulls[Natts_pg_statistic_ext_data] = {0};
int2vector *stxkeys;
Relation statrel;
Relation datarel;
@@ -330,9 +330,6 @@ CreateStatistics(CreateStatsStmt *stmt)
/*
* Everything seems fine, so let's build the pg_statistic_ext tuple.
*/
- memset(values, 0, sizeof(values));
- memset(nulls, false, sizeof(nulls));
-
statoid = GetNewOidWithIndex(statrel, StatisticExtOidIndexId,
Anum_pg_statistic_ext_oid);
values[Anum_pg_statistic_ext_oid - 1] = ObjectIdGetDatum(statoid);
@@ -357,9 +354,6 @@ CreateStatistics(CreateStatsStmt *stmt)
*/
datarel = table_open(StatisticExtDataRelationId, RowExclusiveLock);
- memset(datavalues, 0, sizeof(datavalues));
- memset(datanulls, false, sizeof(datanulls));
Oid namespaceId;
Oid stxowner = GetUserId();
HeapTuple htup;
- Datum values[Natts_pg_statistic_ext];
- bool nulls[Natts_pg_statistic_ext];
- Datum datavalues[Natts_pg_statistic_ext_data];
- bool datanulls[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext] = {0};
+ bool nulls[Natts_pg_statistic_ext] = {0};
+ Datum datavalues[Natts_pg_statistic_ext_data] = {0};
+ bool datanulls[Natts_pg_statistic_ext_data] = {0};
int2vector *stxkeys;
Relation statrel;
Relation datarel;
@@ -330,9 +330,6 @@ CreateStatistics(CreateStatsStmt *stmt)
/*
* Everything seems fine, so let's build the pg_statistic_ext tuple.
*/
- memset(values, 0, sizeof(values));
- memset(nulls, false, sizeof(nulls));
-
statoid = GetNewOidWithIndex(statrel, StatisticExtOidIndexId,
Anum_pg_statistic_ext_oid);
values[Anum_pg_statistic_ext_oid - 1] = ObjectIdGetDatum(statoid);
@@ -357,9 +354,6 @@ CreateStatistics(CreateStatsStmt *stmt)
*/
datarel = table_open(StatisticExtDataRelationId, RowExclusiveLock);
- memset(datavalues, 0, sizeof(datavalues));
- memset(datanulls, false, sizeof(datanulls));
In the first usage, we are initializing the boolean array with 0 and in the second case, we are using false. The patch changes it to use 0 at all the places which I think is better.
I don't have any strong opinion on this, but I would mildly prefer to initialize boolean array with false just for the sake of readability (we generally initializing booleans with false).
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Amit Kapila <amit.kapila16@gmail.com> Sent: Tuesday, 1 October 2019 8:12 PM > +1. This seems like an improvement. I can review and take this forward unless there are objections from others. FYI - I created a Commitfest entry for this here: https://commitfest.postgresql.org/25/2290/ Kind Regards -- Peter Smith Fujitsu Australia
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Amit Kapila <amit.kapila16@gmail.com> Sent: Wednesday, 2 October 2019 9:42 AM > I don't have any strong opinion on this, but I would mildly prefer to initialize boolean array with false just for thesake of readability (we generally initializing booleans with false). Done. Please see attached updated patch. Kind Regards -- Peter Smith Fujitsu Australia
Attachment
Isaac Morland wrote: > I hope you'll forgive a noob question. Why does the "After" > initialization for the boolean array have {0} rather than {false}? I think using a value other than {0} potentially gives the incorrect impression that the value is used for *all* elements of the array/structure, whereas it is only used for the first element. "The remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration." The rest of the elements are being initialized to zero as interpreted by their types (so NULL for pointers, 0.0 for floats, even though neither of them need be bitwise zero). Setting the first item to 0 matches that exactly. Using {false} may encourage the unwary to try bool foo[2] = {true}; which will not set all elements to true.
Joe Nelson <joe@begriffs.com> writes: > Isaac Morland wrote: >> I hope you'll forgive a noob question. Why does the "After" >> initialization for the boolean array have {0} rather than {false}? > I think using a value other than {0} potentially gives the incorrect > impression that the value is used for *all* elements of the > array/structure, whereas it is only used for the first element. There's been something vaguely bothering me about this proposal, and I think you just crystallized it. > Using {false} may encourage the unwary to try > bool foo[2] = {true}; > which will not set all elements to true. Right. I think that in general it's bad practice for an initializer to not specify all fields/elements of the target. It is okay in the specific case that we're substituting for a memset(..., 0, ...). Perhaps we could make this explicit by using a coding style like /* in c.h or some such place: */ #define INIT_ALL_ZEROES {0} /* in code: */ Datum values[N] = INIT_ALL_ZEROES; and then decreeing that it's not project style to use a partial initializer other than in this way. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Isaac Morland
Date:
On Wed, 2 Oct 2019 at 11:34, Joe Nelson <joe@begriffs.com> wrote:
Isaac Morland wrote:
> I hope you'll forgive a noob question. Why does the "After"
> initialization for the boolean array have {0} rather than {false}?
I think using a value other than {0} potentially gives the incorrect
impression that the value is used for *all* elements of the
array/structure, whereas it is only used for the first element. "The
remainder of the aggregate shall be initialized implicitly the same as
objects that have static storage duration."
The rest of the elements are being initialized to zero as interpreted by
their types (so NULL for pointers, 0.0 for floats, even though neither
of them need be bitwise zero). Setting the first item to 0 matches that
exactly.
Using {false} may encourage the unwary to try
bool foo[2] = {true};
which will not set all elements to true.
Thanks for the explanation. So the first however many elements are in curly braces get initialized to those values, then the rest get initialized to blank/0/0.0/false/...?
If so, I don't suppose it's possible to give empty braces:
bool nulls[Natts_pg_attribute] = {};
> If so, I don't suppose it's possible to give empty braces: > > bool nulls[Natts_pg_attribute] = {}; GNU does add this capability as a nonstandard language extension, but according to the C99 standard, no.
On 10/2/19 8:46 AM, Tom Lane wrote: > Joe Nelson <joe@begriffs.com> writes: >> Isaac Morland wrote: >>> I hope you'll forgive a noob question. Why does the "After" >>> initialization for the boolean array have {0} rather than {false}? > >> I think using a value other than {0} potentially gives the incorrect >> impression that the value is used for *all* elements of the >> array/structure, whereas it is only used for the first element. > > There's been something vaguely bothering me about this proposal, > and I think you just crystallized it. > >> Using {false} may encourage the unwary to try >> bool foo[2] = {true}; >> which will not set all elements to true. > > Right. I think that in general it's bad practice for an initializer > to not specify all fields/elements of the target. It is okay in the > specific case that we're substituting for a memset(..., 0, ...). > Perhaps we could make this explicit by using a coding style like > > /* in c.h or some such place: */ > #define INIT_ALL_ZEROES {0} > > /* in code: */ > Datum values[N] = INIT_ALL_ZEROES; > > and then decreeing that it's not project style to use a partial > initializer other than in this way. There are numerous locations in the code that raise warnings when -Wmissing-field-initializers is handed to gcc. See, for example, src/backend/utils/adt/formatting.c where static const KeyWord NUM_keywords[] is initialized, and the code comment above that disclaims the need to initialize is_digit and date_mode. Are you proposing cleaning up all such incomplete initializations within the project? I understand that your INIT_ALL_ZEROS macro does nothing to change whether -Wmissing-field-initializers would raise a warning. I'm just asking about the decree you propose, and I used that warning flag to get the compiler to spit out relevant examples. mark
Mark Dilger <hornschnorter@gmail.com> writes: > On 10/2/19 8:46 AM, Tom Lane wrote: >> Right. I think that in general it's bad practice for an initializer >> to not specify all fields/elements of the target. > There are numerous locations in the code that raise warnings when > -Wmissing-field-initializers is handed to gcc. See, for example, > src/backend/utils/adt/formatting.c where > static const KeyWord NUM_keywords[] > is initialized, and the code comment above that disclaims the need to > initialize is_digit and date_mode. Are you proposing cleaning up all > such incomplete initializations within the project? Hmm. Maybe it's worth doing as a code beautification effort, but I'm not volunteering. At the same time, I wouldn't like to make a change like this, if it introduces dozens/hundreds of new cases. > I understand that your INIT_ALL_ZEROS macro does nothing to change > whether -Wmissing-field-initializers would raise a warning. Not sure --- the name of that option suggests that maybe it only complains about omitted *struct fields* not omitted *array elements*. If it does complain, is there any way that we could extend the macro to annotate usages of it to suppress the warning? regards, tom lane
On 10/2/19 11:02 AM, Tom Lane wrote: > Mark Dilger <hornschnorter@gmail.com> writes: >> On 10/2/19 8:46 AM, Tom Lane wrote: >>> Right. I think that in general it's bad practice for an initializer >>> to not specify all fields/elements of the target. > >> There are numerous locations in the code that raise warnings when >> -Wmissing-field-initializers is handed to gcc. See, for example, >> src/backend/utils/adt/formatting.c where >> static const KeyWord NUM_keywords[] >> is initialized, and the code comment above that disclaims the need to >> initialize is_digit and date_mode. Are you proposing cleaning up all >> such incomplete initializations within the project? > > Hmm. Maybe it's worth doing as a code beautification effort, but > I'm not volunteering. At the same time, I wouldn't like to make a > change like this, if it introduces dozens/hundreds of new cases. > >> I understand that your INIT_ALL_ZEROS macro does nothing to change >> whether -Wmissing-field-initializers would raise a warning. > > Not sure --- the name of that option suggests that maybe it only > complains about omitted *struct fields* not omitted *array elements*. With gcc (Debian 8.3.0-6) 8.3.0 int foo[6] = {0, 1, 2}; does not draw a warning when compiled with this flag. > If it does complain, is there any way that we could extend the macro > to annotate usages of it to suppress the warning? Neither initializing a struct with {0} nor with INIT_ALL_ZEROS draws a warning either, with my gcc. There are reports online that older versions of the compiler did, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36750 but I don't have an older version to test with just now. Note that initializing a multi-element struct with {1} does still draw a warning, and reading the thread above suggests that gcc made a specific effort to allow initialization to {0} to work without warning as a special case. So your proposal for using INIT_ALL_ZEROS is probably good with sufficiently new compilers, and I'm generally in favor of the proposal, but I don't think the decree you propose can work unless somebody cleans up all these other cases that I indicated in my prior email. (I'm sitting on a few patches until v12 goes out the door from some conversations with you several months ago, and perhaps I'll include a patch for this cleanup, too, when time comes for v13 patch sets to be submitted. My past experience submitting patches shortly before a release was that they get ignored.) mark
Mark Dilger <hornschnorter@gmail.com> writes: > (I'm sitting on a few patches until v12 goes out the door from some > conversations with you several months ago, and perhaps I'll include a > patch for this cleanup, too, when time comes for v13 patch sets to be > submitted. That would be now. We already ran one CF for v13. > My past experience submitting patches shortly before a > release was that they get ignored.) What you need to do is add 'em to the commitfest app. They might still get ignored for awhile, but we won't forget about them. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Michael Paquier
Date:
On Wed, Oct 02, 2019 at 02:55:39PM -0400, Tom Lane wrote: > Mark Dilger <hornschnorter@gmail.com> writes: >> (I'm sitting on a few patches until v12 goes out the door from some >> conversations with you several months ago, and perhaps I'll include a >> patch for this cleanup, too, when time comes for v13 patch sets to be >> submitted. > > That would be now. We already ran one CF for v13. +1. >> My past experience submitting patches shortly before a >> release was that they get ignored.) > > What you need to do is add 'em to the commitfest app. They might > still get ignored for awhile, but we won't forget about them. The last commit fest of v13 will begin on March, and the next one is planned for the beginning of November: https://commitfest.postgresql.org/25/ So you still have plently of time to get something into 13. Here are also some guidelines: https://wiki.postgresql.org/wiki/Submitting_a_Patch But you are already aware of anyway, right? :p -- Michael
Attachment
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
-----Original Message----- From: Tom Lane <tgl@sss.pgh.pa.us> Sent: Thursday, 3 October 2019 1:46 AM > Right. I think that in general it's bad practice for an initializer to not specify all fields/elements of the target. > It is okay in the specific case that we're substituting for a memset(..., 0, ...). > Perhaps we could make this explicit by using a coding style like > >/* in c.h or some such place: */ >#define INIT_ALL_ZEROES {0} > >/* in code: */ > Datum values[N] = INIT_ALL_ZEROES; The patch has been updated per your suggestion. Now using macros for these partial initialisers. Please see attachment. Kind Regards --- Peter Smith Fujitsu Australia
Attachment
On Wed, Oct 2, 2019 at 9:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Nelson <joe@begriffs.com> writes:
> Isaac Morland wrote:
>> I hope you'll forgive a noob question. Why does the "After"
>> initialization for the boolean array have {0} rather than {false}?
> I think using a value other than {0} potentially gives the incorrect
> impression that the value is used for *all* elements of the
> array/structure, whereas it is only used for the first element.
There's been something vaguely bothering me about this proposal,
and I think you just crystallized it.
> Using {false} may encourage the unwary to try
> bool foo[2] = {true};
> which will not set all elements to true.
Right. I think that in general it's bad practice for an initializer
to not specify all fields/elements of the target. It is okay in the
specific case that we're substituting for a memset(..., 0, ...).
Perhaps we could make this explicit by using a coding style like
/* in c.h or some such place: */
#define INIT_ALL_ZEROES {0}
/* in code: */
Datum values[N] = INIT_ALL_ZEROES;
This is a good idea, but by reading the thread it is not completely clear if we want to pursue this or want to explore something else or leave the current code as it is. Also, if we want to pursue, do we want to use INIT_ALL_ZEROES for bool arrays as well?
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Amit Kapila <amit.kapila16@gmail.com> Sent: Friday, 4 October 2019 1:32 PM > Also, if we want to pursue, do we want to use INIT_ALL_ZEROES for bool arrays as well? FYI - In case it went unnoticed - my last patch addresses this by defining 2 macros: #define INIT_ALL_ELEMS_ZERO {0} #define INIT_ALL_ELEMS_FALSE {false} Kind Regards -- Peter Smith Fujitsu Australia
"Smith, Peter" <peters@fast.au.fujitsu.com> writes: > From: Amit Kapila <amit.kapila16@gmail.com> Sent: Friday, 4 October 2019 1:32 PM >> Also, if we want to pursue, do we want to use INIT_ALL_ZEROES for bool arrays as well? > FYI - In case it went unnoticed - my last patch addresses this by defining 2 macros: > #define INIT_ALL_ELEMS_ZERO {0} > #define INIT_ALL_ELEMS_FALSE {false} I would say that's 100% wrong. The entire point here is that it's memset-equivalent, and therefore writes zeroes, regardless of what the datatype is. As a counterexample, the coding you have above looks a lot like it would work to add #define INIT_ALL_ELEMS_TRUE {true} which as previously noted will *not* work. So I think the one-size-fits-all approach is what to use. regards, tom lane
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Tom Lane <tgl@sss.pgh.pa.us> Sent: Friday, 4 October 2019 2:08 PM >> #define INIT_ALL_ELEMS_ZERO {0} >> #define INIT_ALL_ELEMS_FALSE {false} >I would say that's 100% wrong. The entire point here is that it's memset-equivalent, and therefore writes zeroes, regardlessof what the datatype is. I agree it is memset-equivalent. All examples of the memset code that INIT_ALL_ELEMS_ZERO replaces looked like this: memset(values, 0, sizeof(values)); Most examples of the memset code that INIT_ALL_ELEMS_FALSE replaces looked like this: memset(nulls, false, sizeof(nulls)); ~ I made the 2nd macro because I anticipate the same folk that don't like setting 0 to a bool will also not like setting somethingcalled INIT_ALL_ELEMS_ZERO to a bool array. How about I just define them both the same? #define INIT_ALL_ELEMS_ZERO {0} #define INIT_ALL_ELEMS_FALSE {0} >As a counterexample, the coding you have above looks a lot like it would work to add > >#define INIT_ALL_ELEMS_TRUE {true} > which as previously noted will *not* work. So I think the one-size-fits-all approach is what to use. I agree it looks that way; in my previous email I should have provided more context to the code. Below is the full fragment of the last shared patch, which included a note to prevent anybody from doing such a thing. ~~ /* * Macros for C99 designated-initialiser syntax to set all array elements to 0/false. * * Use these macros in preference to explicit {0} syntax to avoid giving a misleading * impression that the same value is always used for all elements. * e.g. * bool foo[2] = {false}; // sets both elements false * bool foo[2] = {true}; // does NOT set both elements true * * Reference: C99 [$6.7.8/21] If there are fewer initializers in a brace-enclosed list than there * are elements or members of an aggregate, or fewer characters in a string literal used to * initialize an array of known size than there are elements in the array, the remainder of the * aggregate shall be initialized implicitly the same as objects that have static storage duration */ #define INIT_ALL_ELEMS_ZERO {0} #define INIT_ALL_ELEMS_FALSE {false} ~~ Kind Regards, -- Peter Smith Fujitsu Australia
On Fri, Oct 4, 2019 at 12:10 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote:
From: Tom Lane <tgl@sss.pgh.pa.us> Sent: Friday, 4 October 2019 2:08 PM
>> #define INIT_ALL_ELEMS_ZERO {0}
>> #define INIT_ALL_ELEMS_FALSE {false}
>I would say that's 100% wrong. The entire point here is that it's memset-equivalent, and therefore writes zeroes, regardless of what the datatype is.
I agree it is memset-equivalent.
All examples of the memset code that INIT_ALL_ELEMS_ZERO replaces looked like this:
memset(values, 0, sizeof(values));
Most examples of the memset code that INIT_ALL_ELEMS_FALSE replaces looked like this:
memset(nulls, false, sizeof(nulls));
~
I made the 2nd macro because I anticipate the same folk that don't like setting 0 to a bool will also not like setting something called INIT_ALL_ELEMS_ZERO to a bool array.
How about I just define them both the same?
#define INIT_ALL_ELEMS_ZERO {0}
#define INIT_ALL_ELEMS_FALSE {0}
I think using one define would be preferred, but you can wait and see if others prefer defining different macros for the same thing.
Amit Kapila wrote: > > How about I just define them both the same? > > #define INIT_ALL_ELEMS_ZERO {0} > > #define INIT_ALL_ELEMS_FALSE {0} > > I think using one define would be preferred, but you can wait and see > if others prefer defining different macros for the same thing. +1 on using INIT_ALL_ELEMS_ZERO everywhere, even for bool[]. You may worry, "Should we really be assigning 0 to something of type bool? Is that the wrong type or a detail that varies by implementation?" It's safe though, the behavior is guaranteed to be correct by section 7.16 of the C99 spec, which says that bool, true, and false are always macros for _Bool, 1, and 0 respectively. One might argue that INIT_ALL_ELEMS_FALSE as a synonym for INIT_ALL_ELEMS_ZERO is good for readability in the same way that "false" is for 0. However I want to avoid creating the impression that there is, or can be, a collection of INIT_ALL_ELEMS_xxx macros invoking different initializer behavior.
Joe Nelson <joe@begriffs.com> writes: > One might argue that INIT_ALL_ELEMS_FALSE as a synonym for > INIT_ALL_ELEMS_ZERO is good for readability in the same way that "false" > is for 0. However I want to avoid creating the impression that there is, > or can be, a collection of INIT_ALL_ELEMS_xxx macros invoking different > initializer behavior. I concur with Joe here. The reason why some of the existing memset's use "false" is for symmetry with other places where we use "memset(p, true, n)" to set an array of bools to all-true. That coding is unfortunately a bit dubious --- it would sort-of fail if bool weren't of width 1, in that the bools would still test as true but they wouldn't contain the standard bit pattern for true. I don't want to change those places, but we shouldn't make the mechanism proposed by this patch look like it can do anything but initialize to zeroes. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Jacob Champion
Date:
On Fri, Oct 4, 2019 at 7:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I concur with Joe here. The reason why some of the existing > memset's use "false" is for symmetry with other places where we use > "memset(p, true, n)" to set an array of bools to all-true. That > coding is unfortunately a bit dubious --- it would sort-of fail if > bool weren't of width 1, in that the bools would still test as true > but they wouldn't contain the standard bit pattern for true. > I don't want to change those places, but we shouldn't make the > mechanism proposed by this patch look like it can do anything but > initialize to zeroes. > > regards, tom lane Why introduce a macro at all for the universal zero initializer, if it seems to encourage the construction of other (incorrect) macros? IMO the use of {0} as an initializer is well understood in the C developer community, and I'm used to it showing up verbatim in code. Similar to {}'s role as the C++ universal initializer. --Jacob
Jacob Champion <pchampion@pivotal.io> writes: > On Fri, Oct 4, 2019 at 7:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I concur with Joe here. The reason why some of the existing >> memset's use "false" is for symmetry with other places where we use >> "memset(p, true, n)" to set an array of bools to all-true. > Why introduce a macro at all for the universal zero initializer, if it > seems to encourage the construction of other (incorrect) macros? Well, the argument is that some people might think that if {0} is enough to set all array elements to 0, then maybe {1} sets them all to ones (as, indeed, one could argue would be a far better specification than what the C committee actually wrote). Using a separate macro and then discouraging direct use of the incomplete-initializer syntax should help to avoid that error. > IMO > the use of {0} as an initializer is well understood in the C developer > community, and I'm used to it showing up verbatim in code. Yeah, if we were all 100% familiar with every sentence in the C standard, we could argue like that. But we get lots of submissions from people for whom C is not their main language. The fewer gotchas there are in our agreed-on subset of C, the better. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Ashwin Agrawal
Date:
On Fri, Oct 4, 2019 at 8:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jacob Champion <pchampion@pivotal.io> writes:
> On Fri, Oct 4, 2019 at 7:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I concur with Joe here. The reason why some of the existing
>> memset's use "false" is for symmetry with other places where we use
>> "memset(p, true, n)" to set an array of bools to all-true.
> Why introduce a macro at all for the universal zero initializer, if it
> seems to encourage the construction of other (incorrect) macros?
Well, the argument is that some people might think that if {0} is enough
to set all array elements to 0, then maybe {1} sets them all to ones
(as, indeed, one could argue would be a far better specification than
what the C committee actually wrote). Using a separate macro and then
discouraging direct use of the incomplete-initializer syntax should help
to avoid that error.
Seems avoidable overhead to remind folks on macro existence. Plus, for such a thing macro exist in first place will be hard to remember. So, irrespective in long run, {0} might get used in code and hence seems better to just use {0} from start itself instead of macro/wrapper on top.
Plus, even if someone starts out with thought {1} sets them all to ones, I feel will soon realize by exercising the code isn't the reality. If such code is written and nothing fails, that itself seems bigger issue.
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Chapman Flack
Date:
On 10/4/19 1:44 PM, Ashwin Agrawal wrote: > macro exist in first place will be hard to remember. So, irrespective > in long run, {0} might get used in code and hence seems better > to just use {0} from start itself instead of macro/wrapper on top. > > Plus, even if someone starts out with thought {1} sets them all to ones, > I feel will soon realize by exercising the code isn't the reality. I wish ISO C had gone the same place gcc (and C++ ?) went, and allowed the initializer {}, which would eliminate any chance of it misleading a casual reader. If that were the case, I would be +1 on just using the {} syntax. But given that the standard is stuck on requiring a first element, I am +1 on using the macro, just to avoid giving any wrong impressions, even fleeting ones. Regards, -Chap
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Bruce Momjian
Date:
On Fri, Oct 4, 2019 at 02:05:41PM -0400, Chapman Flack wrote: > On 10/4/19 1:44 PM, Ashwin Agrawal wrote: > > > macro exist in first place will be hard to remember. So, irrespective > > in long run, {0} might get used in code and hence seems better > > to just use {0} from start itself instead of macro/wrapper on top. > > > > Plus, even if someone starts out with thought {1} sets them all to ones, > > I feel will soon realize by exercising the code isn't the reality. > > I wish ISO C had gone the same place gcc (and C++ ?) went, and allowed > the initializer {}, which would eliminate any chance of it misleading > a casual reader. > > If that were the case, I would be +1 on just using the {} syntax. > > But given that the standard is stuck on requiring a first element, > I am +1 on using the macro, just to avoid giving any wrong impressions, > even fleeting ones. Yeah, it is certainly weird that you have to assign the first array element to get the rest to be zeros. By using a macro, we can document this behavior in one place. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
Hi, On 2019-10-04 16:31:29 -0400, Bruce Momjian wrote: > On Fri, Oct 4, 2019 at 02:05:41PM -0400, Chapman Flack wrote: > > On 10/4/19 1:44 PM, Ashwin Agrawal wrote: > > > > > macro exist in first place will be hard to remember. So, irrespective > > > in long run, {0} might get used in code and hence seems better > > > to just use {0} from start itself instead of macro/wrapper on top. It already is in somewhat frequent use, fwiw. > > > Plus, even if someone starts out with thought {1} sets them all to ones, > > > I feel will soon realize by exercising the code isn't the reality. > > > > I wish ISO C had gone the same place gcc (and C++ ?) went, and allowed > > the initializer {}, which would eliminate any chance of it misleading > > a casual reader. > > > > If that were the case, I would be +1 on just using the {} syntax. > > > > But given that the standard is stuck on requiring a first element, > > I am +1 on using the macro, just to avoid giving any wrong impressions, > > even fleeting ones. > > Yeah, it is certainly weird that you have to assign the first array > element to get the rest to be zeros. By using a macro, we can document > this behavior in one place. IDK, to me this seems like something one just has to learn about C, with the macro just obfuscating that already required knowledge. It's not like this only applies to stack variables initializes with {0}. It's also true of global variables, or function-local static ones, for example. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2019-10-04 16:31:29 -0400, Bruce Momjian wrote: >> Yeah, it is certainly weird that you have to assign the first array >> element to get the rest to be zeros. By using a macro, we can document >> this behavior in one place. > IDK, to me this seems like something one just has to learn about C, with > the macro just obfuscating that already required knowledge. It's not > like this only applies to stack variables initializes with {0}. It's > also true of global variables, or function-local static ones, for > example. Huh? For both those cases, the *default* behavior, going back to K&R C, is that the variable initializes to all-bits-zero. There's no need to write anything extra. If some people are writing {0} there, I think we should discourage that on the grounds that it results in inconsistent coding style. Note that I'm not proposing a rule against, say, static MyNodeType *my_variable = NULL; That's perfectly sensible and adds no cognitive load that I can see. But in cases where you have to indulge in type punning or reliance on obscure language features to get the result that would happen if you'd just not written anything, I think you should just not write anything. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
Hi, On 2019-10-04 17:08:29 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-10-04 16:31:29 -0400, Bruce Momjian wrote: > >> Yeah, it is certainly weird that you have to assign the first array > >> element to get the rest to be zeros. By using a macro, we can document > >> this behavior in one place. > > > IDK, to me this seems like something one just has to learn about C, with > > the macro just obfuscating that already required knowledge. It's not > > like this only applies to stack variables initializes with {0}. It's > > also true of global variables, or function-local static ones, for > > example. > > Huh? For both those cases, the *default* behavior, going back to K&R C, > is that the variable initializes to all-bits-zero. There's no need to > write anything extra. What I mean is that if there's any initialization, it's to all zeroes, except for the parts explicitly initialized explicitly. And all that the {0} does, is that the rest of the fields are initialized the way other such initialization happens. There's plenty places where we don't initialize every part, e.g. a struct member, of global variables (and for stack allocated data as well, increasingly so). To be able to make sense of things like somevar = {.foo = bar /* field blub is not initialized */}; or things like guc.c - where we rely on zero initialize most fields of config_generic. > If some people are writing {0} there, I think > we should discourage that on the grounds that it results in inconsistent > coding style. Yea, I'm not advocating that. Greetings, Andres Freund
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
From: Amit Kapila <amit.kapila16@gmail.com> Sent: Friday, 4 October 2019 4:50 PM >>How about I just define them both the same? >>#define INIT_ALL_ELEMS_ZERO {0} >>#define INIT_ALL_ELEMS_FALSE {0} > >I think using one define would be preferred, but you can wait and see if others prefer defining different macros for thesame thing. While nowhere near unanimous, it seems majority favour using a macro (if only to protect the unwary and document the behaviour). And of those in favour of macros, using INIT_ALL_ELEMS_ZERO even for bool array is a clear preference. So, please find attached the updated patch, which now has just 1 macro. Kind Regards -- Peter Smith Fujitsu Australia
Attachment
On Sat, Oct 5, 2019 at 3:10 AM Andres Freund <andres@anarazel.de> wrote: > > On 2019-10-04 17:08:29 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > On 2019-10-04 16:31:29 -0400, Bruce Momjian wrote: > > >> Yeah, it is certainly weird that you have to assign the first array > > >> element to get the rest to be zeros. By using a macro, we can document > > >> this behavior in one place. > > > > > IDK, to me this seems like something one just has to learn about C, with > > > the macro just obfuscating that already required knowledge. It's not > > > like this only applies to stack variables initializes with {0}. It's > > > also true of global variables, or function-local static ones, for > > > example. > > > > Huh? For both those cases, the *default* behavior, going back to K&R C, > > is that the variable initializes to all-bits-zero. There's no need to > > write anything extra. > > What I mean is that if there's any initialization, it's to all zeroes, > except for the parts explicitly initialized explicitly. And all that the > {0} does, is that the rest of the fields are initialized the way other > such initialization happens. > You have a point and I think over time everyone will know this. However, so many people advocating for having a macro with a comment to be more explicit about this behavior shows that this is not equally obvious to everyone or at least they think that it will help future patch authors. Now, I think as the usage ({0}) already exists in the code, so I think if we decide to use a macro, then ideally those places should also be changed. I am not telling that it must be done in the same patch, we can even do it as a separate patch. I am personally still in the camp of people advocating the use of macro for this purpose. It is quite possible after reading your points, some people might change their opinion or some others also share their opinion against using a macro in which case we can drop the idea of using a macro. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Oct 8, 2019 at 4:43 AM Smith, Peter <peters@fast.au.fujitsu.com> wrote: > > From: Amit Kapila <amit.kapila16@gmail.com> Sent: Friday, 4 October 2019 4:50 PM > > >>How about I just define them both the same? > >>#define INIT_ALL_ELEMS_ZERO {0} > >>#define INIT_ALL_ELEMS_FALSE {0} > > > >I think using one define would be preferred, but you can wait and see if others prefer defining different macros for thesame thing. > > While nowhere near unanimous, it seems majority favour using a macro (if only to protect the unwary and document the behaviour). > And of those in favour of macros, using INIT_ALL_ELEMS_ZERO even for bool array is a clear preference. > > So, please find attached the updated patch, which now has just 1 macro. Few thoughts on the patch: --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -770,8 +770,8 @@ pg_prepared_xact(PG_FUNCTION_ARGS) GlobalTransaction gxact = &status->array[status->currIdx++]; PGPROC *proc = &ProcGlobal->allProcs[gxact->pgprocno]; PGXACT *pgxact = &ProcGlobal->allPgXact[gxact->pgprocno]; - Datum values[5]; - bool nulls[5]; + Datum values[5] = INIT_ALL_ELEMS_ZERO; + bool nulls[5] = INIT_ALL_ELEMS_ZERO; HeapTuple tuple; Datum result; Initialisation may not be required here as all the members are getting populated immediately @@ -1314,9 +1314,6 @@ SetDefaultACL(InternalDefaultACL *iacls) Oid defAclOid; /* Prepare to insert or update pg_default_acl entry */ - MemSet(values, 0, sizeof(values)); - MemSet(nulls, false, sizeof(nulls)); - MemSet(replaces, false, sizeof(replaces)); if (isNew) We can place the comment just before the next block of code for better readability like you have done in other places. @@ -2024,9 +2018,6 @@ ExecGrant_Relation(InternalGrant *istmt) nnewmembers = aclmembers(new_acl, &newmembers); /* finished building new ACL value, now insert it */ - MemSet(values, 0, sizeof(values)); - MemSet(nulls, false, sizeof(nulls)); - MemSet(replaces, false, sizeof(replaces)); replaces[Anum_pg_class_relacl - 1] = true; We can place the comment just before the next block of code for better readability like you have done in other places. There are few more instances like this in the same file, we can handle that too. -- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -77,7 +77,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) bool immediately_reserve = PG_GETARG_BOOL(1); bool temporary = PG_GETARG_BOOL(2); Datum values[2]; - bool nulls[2]; + bool nulls[2] = INIT_ALL_ELEMS_ZERO; TupleDesc tupdesc; HeapTuple tuple; Datum result; @@ -95,12 +95,10 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) InvalidXLogRecPtr); values[0] = NameGetDatum(&MyReplicationSlot->data.name); - nulls[0] = false; if (immediately_reserve) { values[1] = LSNGetDatum(MyReplicationSlot->data.restart_lsn); - nulls[1] = false; } else nulls[1] = true; We might not gain much here, may be this change is not required. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Thomas Munro
Date:
On Tue, Oct 8, 2019 at 11:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sat, Oct 5, 2019 at 3:10 AM Andres Freund <andres@anarazel.de> wrote: > > On 2019-10-04 17:08:29 -0400, Tom Lane wrote: > > > Andres Freund <andres@anarazel.de> writes: > > > > On 2019-10-04 16:31:29 -0400, Bruce Momjian wrote: > > > >> Yeah, it is certainly weird that you have to assign the first array > > > >> element to get the rest to be zeros. By using a macro, we can document > > > >> this behavior in one place. > > > > > > > IDK, to me this seems like something one just has to learn about C, with > > > > the macro just obfuscating that already required knowledge. It's not > > > > like this only applies to stack variables initializes with {0}. It's > > > > also true of global variables, or function-local static ones, for > > > > example. > > > > > > Huh? For both those cases, the *default* behavior, going back to K&R C, > > > is that the variable initializes to all-bits-zero. There's no need to > > > write anything extra. > > > > What I mean is that if there's any initialization, it's to all zeroes, > > except for the parts explicitly initialized explicitly. And all that the > > {0} does, is that the rest of the fields are initialized the way other > > such initialization happens. > > > > You have a point and I think over time everyone will know this. > However, so many people advocating for having a macro with a comment > to be more explicit about this behavior shows that this is not equally > obvious to everyone or at least they think that it will help future > patch authors. > > Now, I think as the usage ({0}) already exists in the code, so I think > if we decide to use a macro, then ideally those places should also be > changed. I am not telling that it must be done in the same patch, we > can even do it as a separate patch. > > I am personally still in the camp of people advocating the use of > macro for this purpose. It is quite possible after reading your > points, some people might change their opinion or some others also > share their opinion against using a macro in which case we can drop > the idea of using a macro. -1 for these macros. These are basic facts about the C language. I hope C eventually supports {} like C++, so that you don't have to think hard about whether the first member is another struct, and recursively so … but since the macros can't help with that problem, what is the point? I am reminded of an (apocryphal?) complaint from an old C FAQ about people using #define BEGIN {.
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Michael Paquier
Date:
On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote: > -1 for these macros. > > These are basic facts about the C language. I hope C eventually > supports {} like C++, so that you don't have to think hard about > whether the first member is another struct, and recursively so … but > since the macros can't help with that problem, what is the point? FWIW, I am not convinced that those macros are an improvement either. > I am reminded of an (apocryphal?) complaint from an old C FAQ about > people using #define BEGIN {. This one? Wow. http://c-faq.com/cpp/slm.html -- Michael
Attachment
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Kyotaro Horiguchi
Date:
At Thu, 17 Oct 2019 16:30:02 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote: > > -1 for these macros. > > > > These are basic facts about the C language. I hope C eventually > > supports {} like C++, so that you don't have to think hard about > > whether the first member is another struct, and recursively so … but > > since the macros can't help with that problem, what is the point? > > FWIW, I am not convinced that those macros are an improvement either. FWIW agreed. I might have put +1 if it had multpile definitions according to platforms, though. > > I am reminded of an (apocryphal?) complaint from an old C FAQ about > > people using #define BEGIN {. > > This one? Wow. > http://c-faq.com/cpp/slm.html I remember this. Though the new macro proposed here doesn't completely seems to be a so-called nonsyntactic macro, but the syntax using the macro looks somewhat broken since it lacks {}, which should be there. bool nulls[Natts_pg_collection] = INIT_ALL_ELEMS_ZERO; We could abuse the macro for structs. pgstattuple_type stat = INIT_ALL_ELEMS_ZERO; This is correct in syntax, but seems completely broken. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Oct 17, 2019 at 4:58 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 17 Oct 2019 16:30:02 +0900, Michael Paquier <michael@paquier.xyz> wrote in > > On Thu, Oct 17, 2019 at 06:37:11PM +1300, Thomas Munro wrote: > > > -1 for these macros. > > > > > > These are basic facts about the C language. I hope C eventually > > > supports {} like C++, so that you don't have to think hard about > > > whether the first member is another struct, and recursively so … but > > > since the macros can't help with that problem, what is the point? > > > > FWIW, I am not convinced that those macros are an improvement either. > > FWIW agreed. I might have put +1 if it had multpile definitions > according to platforms, though. > Thanks, Thomas, Michael, and Horiguchi-San. I think there are enough votes on not using a macro that we can proceed with that approach. This takes us back to what Smith, Peter has initially proposed [1]. I shall wait for a couple of days to see if someone would like to argue otherwise and then review the proposed patch. [1] - https://www.postgresql.org/message-id/201DD0641B056142AC8C6645EC1B5F62014B919631%40SYD1217 -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Stephen Frost
Date:
Greetings, * Thomas Munro (thomas.munro@gmail.com) wrote: > On Tue, Oct 8, 2019 at 11:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > I am personally still in the camp of people advocating the use of > > macro for this purpose. It is quite possible after reading your > > points, some people might change their opinion or some others also > > share their opinion against using a macro in which case we can drop > > the idea of using a macro. > > -1 for these macros. Agreed. > These are basic facts about the C language. I hope C eventually > supports {} like C++, so that you don't have to think hard about > whether the first member is another struct, and recursively so … but > since the macros can't help with that problem, what is the point? I realize that I need to don some fireproof gear for suggesting this, but I really wonder how much fallout we'd have from just allowing {} to be used.. It's about a billion[1] times cleaner and more sensible than using {0} and doesn't create a dependency on what the first element of the struct is.. Thanks, Stephen 1: Detailed justification not included intentionally and is left as an exercise to the reader.
Attachment
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Chapman Flack
Date:
On 10/18/19 08:18, Stephen Frost wrote: > I realize that I need to don some fireproof gear for suggesting this, > but I really wonder how much fallout we'd have from just allowing {} to > be used.. It's about a billion[1] times cleaner and more sensible than > using {0} and doesn't create a dependency on what the first element of > the struct is.. I guess the non-flamey empirical question would be, if it's not ISO C, are we supporting any compiler that doesn't understand it? -Chap
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Stephen Frost
Date:
Greetings, * Chapman Flack (chap@anastigmatix.net) wrote: > On 10/18/19 08:18, Stephen Frost wrote: > > I realize that I need to don some fireproof gear for suggesting this, > > but I really wonder how much fallout we'd have from just allowing {} to > > be used.. It's about a billion[1] times cleaner and more sensible than > > using {0} and doesn't create a dependency on what the first element of > > the struct is.. > > I guess the non-flamey empirical question would be, if it's not ISO C, > are we supporting any compiler that doesn't understand it? Right, that's basically what I was trying to ask. :) Thanks, Stephen
Attachment
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
Hi, On 2019-10-18 09:03:31 -0400, Stephen Frost wrote: > * Chapman Flack (chap@anastigmatix.net) wrote: > > On 10/18/19 08:18, Stephen Frost wrote: > > > I realize that I need to don some fireproof gear for suggesting this, > > > but I really wonder how much fallout we'd have from just allowing {} to > > > be used.. It's about a billion[1] times cleaner and more sensible than > > > using {0} and doesn't create a dependency on what the first element of > > > the struct is.. > > > > I guess the non-flamey empirical question would be, if it's not ISO C, > > are we supporting any compiler that doesn't understand it? > > Right, that's basically what I was trying to ask. :) I don't understand why this is an issue worth deviating from the standard for. Especially not when the person suggesting to do so isn't even doing the leg work to estimate the portability issues. I feel we've spent more than enough time on this topic. - Andres
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Stephen Frost
Date:
Greetings, * Andres Freund (andres@anarazel.de) wrote: > On 2019-10-18 09:03:31 -0400, Stephen Frost wrote: > > * Chapman Flack (chap@anastigmatix.net) wrote: > > > On 10/18/19 08:18, Stephen Frost wrote: > > > > I realize that I need to don some fireproof gear for suggesting this, > > > > but I really wonder how much fallout we'd have from just allowing {} to > > > > be used.. It's about a billion[1] times cleaner and more sensible than > > > > using {0} and doesn't create a dependency on what the first element of > > > > the struct is.. > > > > > > I guess the non-flamey empirical question would be, if it's not ISO C, > > > are we supporting any compiler that doesn't understand it? > > > > Right, that's basically what I was trying to ask. :) > > I don't understand why this is an issue worth deviating from the > standard for. Because this use and the way the standard is defined in this case is confusing and could lead later hackers to misunderstand what's going on and end up creating bugs- which is what a good chunk of this discussion was about. The {} construct is much clearer in this regard and while it's not in the C standard it's in C++ and it's accepted by the commonly used compilers (clang and and pretty far back it seems for gcc), without warning unless you enable -pedantic or similar. > Especially not when the person suggesting to do so isn't > even doing the leg work to estimate the portability issues. I figured it was common knowledge that gcc/clang supported it just fine, which covers something like 90% of the buildfarm. I haven't got easy access to check others. Thanks, Stephen
Attachment
On Sat, Oct 19, 2019 at 9:14 PM Stephen Frost <sfrost@snowman.net> wrote: > > * Andres Freund (andres@anarazel.de) wrote: > > > Especially not when the person suggesting to do so isn't > > even doing the leg work to estimate the portability issues. > > I figured it was common knowledge that gcc/clang supported it just fine, > which covers something like 90% of the buildfarm. I haven't got easy > access to check others. > I have tried {} on Windows (MSVC-2017) and it is giving compilation error: >\src\backend\access\transam\commit_ts.c(425): error C2059: syntax error: '}' 1>\src\backend\access\transam\commit_ts.c(426): error C2059: syntax error: '}' The changed code looks like below: Datum pg_last_committed_xact(PG_FUNCTION_ARGS) { .. Datum values[2] = {}; bool nulls[2] = {}; .. } Does this put an end to the option of using {} or do we want to investigate something more? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From
"Joe Nelson"
Date:
> > I don't understand why this is an issue worth deviating from the > > standard for. > > Because this use and the way the standard is defined in this case is > confusing and could lead later hackers to misunderstand what's going on > and end up creating bugs- The two possible misunderstandings seem to be: 1. how 0 is interpreted in various contexts such as bool 2. that the x in {x} applies to only the first element IMHO we should expect people to be familiar with (1), and we have the INIT_ALL_ELEMS_ZERO macro to avoid (2). However the more I look at the code using that macro the less I like it. The {0} initializer is more idiomatic. My vote would be to use {0} everywhere and avoid constructions like {false} which might exacerbate misunderstanding (2). > I figured it was common knowledge that gcc/clang supported it just fine, > which covers something like 90% of the buildfarm. I haven't got easy > access to check others. As Amit pointed out, {} doesn't work with MSVC-2017, nor is there any reason it should, given that it isn't part of the C standard.
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Chapman Flack
Date:
On 10/21/19 11:25 AM, Joe Nelson wrote: > we have the > INIT_ALL_ELEMS_ZERO macro to avoid (2). However the more I look at the > code using that macro the less I like it. The {0} initializer is more > idiomatic. If faced with the two questions: 1. which of a or b is more "clear" ? 2. which of a or b is more "idiomatic" ? I think I would feel on more solid ground opining on (1), where wrt (2) I would feel a little muzzier trying to say what the question means. It seems to me that idioms are common bits of usage that take off because they're widely recognized as saying a specific thing efficiently and clearly. On that score, I'm not sure {0} really makes a good idiom ... indeed, it seems this conversation is largely about whether it /looks/ too much like an idiom, and to some readers could appear to be saying something efficiently and clearly but that isn't quite what it means. I would favor {} in a heartbeat if it were standard, because that sucker is an idiom. Failing that, though, I think I still favor the macro, because question (1) seems less fuzzy than question (2), and on "clear", the macro wins. Regards, -Chap
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Isaac Morland
Date:
On Mon, 21 Oct 2019 at 11:46, Chapman Flack <chap@anastigmatix.net> wrote:
I would favor {} in a heartbeat if it were standard, because that
sucker is an idiom.
Failing that, though, I think I still favor the macro, because
question (1) seems less fuzzy than question (2), and on "clear",
the macro wins.
Is it possible to define the macro to be {} where supported and {0} where needed? Something like:
#if ...
#define INIT_ALL_ELEMS_ZERO {}
#else
#define INIT_ALL_ELEMS_ZERO {0}
#endif
Then it's clear the 0 is just there to make certain compilers happy and doesn't have any actual meaning.
Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From
"Joe Nelson"
Date:
> Is it possible to define the macro to be {} where supported and {0} > where needed? Something like: If it's being put behind a macro then *stylistically* it shouldn't matter whether {} or {0} is chosen, right? In which case {0} would be a better choice because it's supported everywhere.
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Stephen Frost
Date:
Greetings, * Joe Nelson (joe@begriffs.com) wrote: > > Is it possible to define the macro to be {} where supported and {0} > > where needed? Something like: > > If it's being put behind a macro then *stylistically* it shouldn't > matter whether {} or {0} is chosen, right? In which case {0} would > be a better choice because it's supported everywhere. The problem with {0} in the first place is that it doesn't actually work in all cases... Simple cases, yes, but not more complex ones. It's unfortunate that there isn't a general solution here that works across platforms (even if it involved macros..), but that seems to be the case. Thanks, Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes: > * Joe Nelson (joe@begriffs.com) wrote: >> If it's being put behind a macro then *stylistically* it shouldn't >> matter whether {} or {0} is chosen, right? In which case {0} would >> be a better choice because it's supported everywhere. > The problem with {0} in the first place is that it doesn't actually work > in all cases... Simple cases, yes, but not more complex ones. It's > unfortunate that there isn't a general solution here that works across > platforms (even if it involved macros..), but that seems to be the case. There is a general solution that works across platforms; it's called memset() and it's what we're using today. I'm beginning to think that we should just reject this patch. It's certainly not enough of an improvement to justify the amount of discussion that's gone into it. regards, tom lane
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
On 2019-10-21 15:04:36 -0400, Tom Lane wrote: > There is a general solution that works across platforms; it's called > memset() and it's what we're using today. I'm beginning to think that > we should just reject this patch. It's certainly not enough of an > improvement to justify the amount of discussion that's gone into it. bikeshedding vs reality of programming & efficiency: 1 : 0.
On Tue, Oct 22, 2019 at 12:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Stephen Frost <sfrost@snowman.net> writes: > > * Joe Nelson (joe@begriffs.com) wrote: > >> If it's being put behind a macro then *stylistically* it shouldn't > >> matter whether {} or {0} is chosen, right? In which case {0} would > >> be a better choice because it's supported everywhere. > > > The problem with {0} in the first place is that it doesn't actually work > > in all cases... Simple cases, yes, but not more complex ones. It's > > unfortunate that there isn't a general solution here that works across > > platforms (even if it involved macros..), but that seems to be the case. > > There is a general solution that works across platforms; it's called > memset() and it's what we're using today. I'm beginning to think that > we should just reject this patch. > Hmm, but then what is your suggestion for existing code that uses {0}. If we reject this patch and leave the current code as it is, there is always a risk of some people using {0} and others using memset which will lead to further deviation in the code. Now, maybe if we change the existing code to always use memset where we use {0}, then we can kind of enforce such a rule for future patch authors. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Michael Paquier
Date:
On Tue, Oct 22, 2019 at 04:13:03PM +0530, Amit Kapila wrote: > Hmm, but then what is your suggestion for existing code that uses {0}. > If we reject this patch and leave the current code as it is, there is > always a risk of some people using {0} and others using memset which > will lead to further deviation in the code. Now, maybe if we change > the existing code to always use memset where we use {0}, then we can > kind of enforce such a rule for future patch authors. Well, we could have a shot at reducing the footprint of {0} then where we can. I am seeing less than a dozen in contrib/, and a bit more than thirty in src/backend/. Or we could just do as we do with such business: let's update them when we see that's adapted and when modifying the surrounding area. At least I see one conclusion coming out of this thread: the patch is in the direction of getting rejected. My recommendation would be to do that, and focus on other patches which could get merged: we have a total of 220 entries in this CF. -- Michael
Attachment
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Andres Freund
Date:
Hi, On 2019-11-12 14:17:42 +0900, Michael Paquier wrote: > On Tue, Oct 22, 2019 at 04:13:03PM +0530, Amit Kapila wrote: > > Hmm, but then what is your suggestion for existing code that uses {0}. > > If we reject this patch and leave the current code as it is, there is > > always a risk of some people using {0} and others using memset which > > will lead to further deviation in the code. Now, maybe if we change > > the existing code to always use memset where we use {0}, then we can > > kind of enforce such a rule for future patch authors. > > Well, we could have a shot at reducing the footprint of {0} then where > we can. I am seeing less than a dozen in contrib/, and a bit more > than thirty in src/backend/. -many. I think this serves zero positive purpose, except to make it harder to analyze code-flow. I think it's not worth going around to convert code to use {0} style initializers in most cases, but when authors write it, we shouldn't remove it either. Greetings, Andres Freund
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
"Smith, Peter"
Date:
Hi Hackers. This submission seems to have stalled. Please forgive this post - I am unsure if the submission process expects me to come to defence of my own patch for one lastgasp, or if I am supposed to just sit back and watch it die a slow death of a thousand cuts. I thought this submission actually started out very popular, but then support slowly eroded, and currently seems headed towardsa likely rejection. ~ Anyway, here are my arguments: (a) I recognise that on first glance, the {0} syntax might evoke a momentary "double-take" by the someone reading the code.IMO this would only be experienced by somebody encountering {0} syntax for the very first time. This is not an reallyuncommon "pattern" (it's already elsewhere in PostreSQL code), and once you've seen it two or three times there isno confusion what it is doing. (b) Because of (a) I don't really agree with the notion that it should be replaced by a macro to hide the C syntax. I didtry adding various macros as suggested, but all that achieved was to was spin off another 20 emails debating the macroformat. I thought any code committer/reviewer should have no trouble at all to understand standard C syntax. (c) It was never a goal of this submission that *all* memsets should be replaced by {0}. Sometimes {0} is more concise andbetter IMO, but sometimes memset is a way more appropriate choice. This patch only replaces simple examples of primitivetypes like the values[] and nulls[] arrays (which was a repeated pattern for many tuple operations). I think anyconcern that {0} may not work for all other complex cases is a red-herring. When memset is better, then use memset. (d) Wishing for C99 syntax to be same as the simpler {} style of C++ is another red-herring. I can only use what is officiallysupported. It is what it is. (e) The PostgreSQL miscellaneous coding conventions - https://www.postgresql.org/docs/current/source-conventions.html - saysto avoid " intermingled declarations and code". This leads to some code where the variable declaration and the initialization(e.g. memset 0 or memset false) code can be widely separated. It can be an easy source of mistakes to assumea variable was already initialized when maybe it wasn't. This patch puts the initialization at the point of declaration,and so eliminates this risk. Isn't that best practice? (f) I'm still a bit perplexed how can it be that removing 200 lines of unnecessary function calls is not considered a goodthing to do? Are patches that only tidy up code generally not accepted? I don't know. ~ That's all I have to say in support of my patch; it will live or it will die according to the community wish. If nothing else, at least I've learned a new term - "bike shedding" :-) Kind Regards. --- Peter Smith Fujitsu Australia
Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays
From
Michael Paquier
Date:
On Thu, Nov 21, 2019 at 04:50:22AM +0000, Smith, Peter wrote: > I thought this submission actually started out very popular, but > then support slowly eroded, and currently seems headed towards a > likely rejection. Yeah, it seems to me that this tends to be a rejection, and the thread has actually died. As we are close to the end of the CF, I am just updating the patch as such. -- Michael