Quick Links

• Community
• Contributors
• Mailing Lists
• IRC
• Local User Groups
• Events
• International Sites

Commit e4602483e95 accidentally introduced a situation where pgindent
disagrees with the git whitespace check. The code is

conn = libpqsrv_connect_params(keywords, values,
/* expand_dbname = */ false,
PG_WAIT_EXTENSION);

where the current source file has 4 spaces before the /*, and the
whitespace check says that that should be a tab.

I think it should actually be 3 spaces, so that the "/*..." lines up
with the "keywords..." and "PG_WAIT..." above and below.

I suppose this isn't going to be a quick fix in pgindent, but if someone
is keeping track, maybe this could be added to the to-consider list.

In the meantime, I suggest we work around this, perhaps by

conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false,
PG_WAIT_EXTENSION);

which appears to be robust for both camps.

On 2023-Feb-22, Peter Eisentraut wrote:

> In the meantime, I suggest we work around this, perhaps by
>
> conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false,
> PG_WAIT_EXTENSION);

I suggest

conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
PG_WAIT_EXTENSION);

which is what we typically do elsewhere and doesn't go overlength.

--
Álvaro Herrera 48°01'N 7°57'E —https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> Commit e4602483e95 accidentally introduced a situation where pgindent
> disagrees with the git whitespace check. The code is

> conn = libpqsrv_connect_params(keywords, values,
> /* expand_dbname = */ false,
> PG_WAIT_EXTENSION);

> where the current source file has 4 spaces before the /*, and the
> whitespace check says that that should be a tab.

Hmm, I don't think that's per project style in the first place.
Most places that annotate function arguments do it like

conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
PG_WAIT_EXTENSION);

pgindent has never been very kind to non-end-of-line comments, and
I'm not excited about working on making it do so. As a thought
experiment, what would happen if we reversed course and started
allowing "//" comments? Naive conversion of this comment could
break the code altogether. (Plenty of programming languages
don't even *have* non-end-of-line comments.)

regards, tom lane

On 2023-02-22 We 09:52, Tom Lane wrote:
> pgindent has never been very kind to non-end-of-line comments, and
> I'm not excited about working on making it do so. As a thought
> experiment, what would happen if we reversed course and started
> allowing "//" comments? Naive conversion of this comment could
> break the code altogether. (Plenty of programming languages
> don't even *have* non-end-of-line comments.)
>
>

I suspect not allowing // is at least a minor annoyance to any new
developer we acquire under the age of about 40.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> I suspect not allowing // is at least a minor annoyance to any new
developer we acquire under the age of about 40.

pgindent changes those to our style, so it's not much of an annoyance if
one prefers to type it that way during development.

--
John Naylor
EDB:http://www.enterprisedb.com

John Naylor <john(dot)naylor(at)enterprisedb(dot)com> writes:
> On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> I suspect not allowing // is at least a minor annoyance to any new
>> developer we acquire under the age of about 40.

> pgindent changes those to our style, so it's not much of an annoyance if
> one prefers to type it that way during development.

Right, it's not like we reject patches for that (or at least, we shouldn't
reject patches for any formatting issues that pgindent can fix).

For my own taste, I really don't have any objection to // in isolation --
the problem with it is just that we've got megabytes of code in the other
style. I fear it'd look really ugly to have an intermixture of // and /*
comment styles. Mass conversion of /* to // style would answer that,
but would also create an impossible back-patching problem.

regards, tom lane

> On 23 Feb 2023, at 05:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> For my own taste, I really don't have any objection to // in isolation --
> the problem with it is just that we've got megabytes of code in the other
> style. I fear it'd look really ugly to have an intermixture of // and /*
> comment styles.

We could use the "use the style of surrounding code (comments)" approach - when
changing an existing commented function use the style already present; when
adding a net new function a choice can be made (unless we mandate a style). It
will still look ugly, but it will be less bad than mixing within the same
block.

> Mass conversion of /* to // style would answer that,
> but would also create an impossible back-patching problem.

Yeah, that sounds incredibly invasive.

--
Daniel Gustafsson

On 2023-02-22 We 23:48, Tom Lane wrote:
> For my own taste, I really don't have any objection to // in isolation --
> the problem with it is just that we've got megabytes of code in the other
> style. I fear it'd look really ugly to have an intermixture of // and /*
> comment styles.

Maybe, I've seen some mixing elsewhere and it didn't make me shudder. I
agree that you probably wouldn't want to mix both styles for end of line
comments in a single function, although a rule like that would be hard
to enforce mechanically.

> Mass conversion of /* to // style would answer that,
> but would also create an impossible back-patching problem.
>
>

Yeah, I agree that's a complete non-starter.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 22.02.23 15:49, Alvaro Herrera wrote:
> On 2023-Feb-22, Peter Eisentraut wrote:
>
>> In the meantime, I suggest we work around this, perhaps by
>>
>> conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false,
>> PG_WAIT_EXTENSION);
>
> I suggest
>
> conn = libpqsrv_connect_params(keywords, values,
> false, /* expand_dbname */
> PG_WAIT_EXTENSION);
>
> which is what we typically do elsewhere and doesn't go overlength.

Fixed this way.

On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote:
> > On 23 Feb 2023, at 05:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > For my own taste, I really don't have any objection to // in isolation --
> > the problem with it is just that we've got megabytes of code in the other
> > style. I fear it'd look really ugly to have an intermixture of // and /*
> > comment styles.
>
> We could use the "use the style of surrounding code (comments)" approach - when
> changing an existing commented function use the style already present; when
> adding a net new function a choice can be made (unless we mandate a style). It
> will still look ugly, but it will be less bad than mixing within the same
> block.
>
> > Mass conversion of /* to // style would answer that,
> > but would also create an impossible back-patching problem.
>
> Yeah, that sounds incredibly invasive.

I am replying late here but ...

We would have to convert all supported branches, and tell all forks to
do the same (hopefully at the same time). The new standard would then
be for all single-line comments to use // instead of /* ... */.

--
Bruce Momjian <bruce(at)momjian(dot)us>https://momjian.us
EDBhttps://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

> On 29 Mar 2023, at 19:18, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote:
>>> On 23 Feb 2023, at 05:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>>> Mass conversion of /* to // style would answer that,
>>> but would also create an impossible back-patching problem.
>>
>> Yeah, that sounds incredibly invasive.
>
> I am replying late here but ...
>
> We would have to convert all supported branches, and tell all forks to
> do the same (hopefully at the same time). The new standard would then
> be for all single-line comments to use // instead of /* ... */.

That still leaves every patch which is in flight on -hackers, and conflicts in
local development trees etc. It's doable (apart from forks, but that cannot be
our core concern), but I personally can't see the price paid justify the result.

--
Daniel Gustafsson

On Wed, Mar 29, 2023 at 08:26:23PM +0200, Daniel Gustafsson wrote:
> > On 29 Mar 2023, at 19:18, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > We would have to convert all supported branches, and tell all forks to
> > do the same (hopefully at the same time). The new standard would then
> > be for all single-line comments to use // instead of /* ... */.
>
> That still leaves every patch which is in flight on -hackers, and conflicts in
> local development trees etc. It's doable (apart from forks, but that cannot be
> our core concern), but I personally can't see the price paid justify the result.

Yes, this would have to be done at the start of a new release cycle.

--
Bruce Momjian <bruce(at)momjian(dot)us>https://momjian.us
EDBhttps://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.