RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays

From: "Smith, Peter" <peters(at)fast(dot)au(dot)fujitsu(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Joe Nelson <joe(at)begriffs(dot)com>, "Isaac Morland" <isaac(dot)morland(at)gmail(dot)com>, Chapman Flack <chap(at)anastigmatix(dot)net>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
Date: 2019-11-21 04:50:22
Message-ID: 201DD0641B056142AC8C6645EC1B5F62014B98E4E6@SYD1217
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 last gasp, 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 towards a 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 really uncommon "pattern" (it's already elsewhere in PostreSQL code), and once you've seen it two or three times there is no 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 did try adding various macros as suggested, but all that achieved was to was spin off another 20 emails debating the macro format. 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 and better IMO, but sometimes memset is a way more appropriate choice. This patch only replaces simple examples of primitive types like the values[] and nulls[] arrays (which was a repeated pattern for many tuple operations). I think any concern 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 officially supported. It is what it is.

(e) The PostgreSQL miscellaneous coding conventions - https://www.postgresql.org/docs/current/source-conventions.html - says to 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 assume a 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 good thing 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2019-11-21 04:56:52 Re: logical decoding : exceeded maxAllocatedDescs for .spill files
Previous Message Tom Lane 2019-11-21 04:45:21 Re: an OID >= 8000 in master