pgindent vs dtrace on macos

On 20 May 2020, at 11:52, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

Or should this file be excluded, since it's generated?

That would get my vote. Generated files where we don't control the generator
can be excluded.

cheers ./daniel

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

If I run pgindent on a built tree on macos, I get this error
Failure in ./src/backend/utils/probes.h: Error@375: Stuff missing from
end of file
The file in question is built by the dtrace command. I have attached it
here.
Is this something to fix in pgindent? Or should this file be excluded,
since it's generated?

Hm, there's nothing obviously wrong with the file. But since it's
generated by code not under our control, we should exclude it.
And given that, it's probably not worth figuring out why it breaks
pgindent.

On a closely related point: I was confused for awhile on Monday
afternoon, wondering why the built tarballs didn't match my local
tree. I eventually realized that when I'd run pgindent on Saturday,
it had reformatted some generated files such as
src/bin/psql/sql_help.h, causing those not to match the freshly-made
ones in the tarball. I wonder if we should make an effort to ensure
that our generated .h and .c files always satisfy pgindent. If not,
we probably should exclude them too.

regards, tom lane

On 2020-05-20 15:56, Tom Lane wrote:

On a closely related point: I was confused for awhile on Monday
afternoon, wondering why the built tarballs didn't match my local
tree. I eventually realized that when I'd run pgindent on Saturday,
it had reformatted some generated files such as
src/bin/psql/sql_help.h, causing those not to match the freshly-made
ones in the tarball. I wonder if we should make an effort to ensure
that our generated .h and .c files always satisfy pgindent.

We should generally try to do that, if only so that they don't appear
weird and random when looking at them.

I think in the past it would have been very difficult for a generation
script to emulate pgindent's weird un-indentation logic on trailing
lines, but that shouldn't be a problem anymore.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 22 May 2020, at 11:29, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 2020-05-20 15:56, Tom Lane wrote:

I wonder if we should make an effort to ensure
that our generated .h and .c files always satisfy pgindent.

We should generally try to do that, if only so that they don't appear weird and random when looking at them.

The attached patch fixes the generation of sql_help.h and perl_opmask.h to make
sure they conform to pgindent. Those were the only file I got diffs in after a
pgindent run apart from fmgrprotos.h which gave the below:

@@ -912,7 +912,7 @@
 extern Datum interval_mul(PG_FUNCTION_ARGS);
 extern Datum pg_typeof(PG_FUNCTION_ARGS);
 extern Datum ascii(PG_FUNCTION_ARGS);
-extern Datum chr(PG_FUNCTION_ARGS);
+extern Datum chr (PG_FUNCTION_ARGS);
 extern Datum repeat(PG_FUNCTION_ARGS);
 extern Datum similar_escape(PG_FUNCTION_ARGS);
 extern Datum mul_d_interval(PG_FUNCTION_ARGS);
@@ -968,7 +968,7 @@
 extern Datum bitsubstr_no_len(PG_FUNCTION_ARGS);
 extern Datum numeric_in(PG_FUNCTION_ARGS);
 extern Datum numeric_out(PG_FUNCTION_ARGS);
-extern Datum numeric(PG_FUNCTION_ARGS);
+extern Datum numeric (PG_FUNCTION_ARGS);
 extern Datum numeric_abs(PG_FUNCTION_ARGS);
 extern Datum numeric_sign(PG_FUNCTION_ARGS);
 extern Datum numeric_round(PG_FUNCTION_ARGS);

Not sure what pgindent is doing there, but it seems hard to address in the
generator.

probes.h is also added to the exclusion list in the patch. On that note, I
wonder if we should add the plperl .xs generated files as exclusions too since
we don't control that generator?

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:

The attached patch fixes the generation of sql_help.h and perl_opmask.h to make
sure they conform to pgindent. Those were the only file I got diffs in after a
pgindent run apart from fmgrprotos.h which gave the below:

Hmm, I seem to recall there were more when this happened to me back in
May. But in any case, fixing these is an improvement.

Not sure what pgindent is doing there, but it seems hard to address in the
generator.

I think the issue is that pgindent believes "numeric" and "chr" are
typedefs. (The regex code can be blamed for "chr", but I'm not quite
sure where "numeric" is coming from.) Observe that it also messes up
the definitions of those two functions, not only their extern
declarations.

We could try adding those names to the typedef exclusion list in pgindent,
but that could easily make things worse not better overall. On balance
I'd say this particular behavior is a pgindent bug, and if anybody is hot
to remove the discrepancy then they ought to try to fix pgindent not the
fmgrprotos generator.

probes.h is also added to the exclusion list in the patch.

Check.

On that note, I
wonder if we should add the plperl .xs generated files as exclusions too since
we don't control that generator?

Not an issue I don't think; pgindent won't touch extensions other than
.c and .h.

regards, tom lane

On 2020-Sep-16, Tom Lane wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

Not sure what pgindent is doing there, but it seems hard to address in the
generator.

I think the issue is that pgindent believes "numeric" and "chr" are
typedefs. (The regex code can be blamed for "chr", but I'm not quite
sure where "numeric" is coming from.)

It's in src/interfaces/ecpg/include/pgtypes_numeric.h

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

The attached patch fixes the generation of sql_help.h and perl_opmask.h to make
sure they conform to pgindent. Those were the only file I got diffs in after a
pgindent run apart from fmgrprotos.h which gave the below:

Hmm, I seem to recall there were more when this happened to me back in
May. But in any case, fixing these is an improvement.

Experimenting with this patch soon found one additional case: sql_help.c,
also emitted by create_help.pl, also needs some whitespace help.
I do not recall if there are other places, but fixing these is
surely a step forward. I fixed the sql_help.c output and pushed it.

regards, tom lane

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2020-Sep-16, Tom Lane wrote:

I think the issue is that pgindent believes "numeric" and "chr" are
typedefs. (The regex code can be blamed for "chr", but I'm not quite
sure where "numeric" is coming from.)

It's in src/interfaces/ecpg/include/pgtypes_numeric.h

It strikes me that a low-cost workaround would be to rename these
C functions. There's no law that their C names must match the
SQL names.

regards, tom lane

The attached patch fixes the generation of sql_help.h and perl_opmask.h to make
sure they conform to pgindent. Those were the only file I got diffs in after a
pgindent run apart from fmgrprotos.h which gave the below:

Hmm, I seem to recall there were more when this happened to me back in
May. But in any case, fixing these is an improvement.

Experimenting with this patch soon found one additional case: sql_help.c,
also emitted by create_help.pl, also needs some whitespace help.
I do not recall if there are other places, but fixing these is
surely a step forward. I fixed the sql_help.c output and pushed it.

Thanks! I think a bug for .c and .h files with matching names in my small
script testing for discrepancies hid that one.

On that note, I
wonder if we should add the plperl .xs generated files as exclusions too since
we don't control that generator?

Not an issue I don't think; pgindent won't touch extensions other than
.c and .h.

Sorry for being unclear, I meant the generated .c counterpart of the .xs file.
So something like the below:

--- a/src/tools/pgindent/exclude_file_patterns
+++ b/src/tools/pgindent/exclude_file_patterns
@@ -5,6 +5,8 @@
 /ecpg/test/expected/
 /snowball/libstemmer/
 /pl/plperl/ppport\.h$
+/pl/plperl/SPI\.c$
+/pl/plperl/Util\.c$
 /jit/llvmjit\.h$
 /utils/probes\.h$
 /tmp_check/

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:

On that note, I
wonder if we should add the plperl .xs generated files as exclusions too since
we don't control that generator?

Not an issue I don't think; pgindent won't touch extensions other than
.c and .h.

Sorry for being unclear, I meant the generated .c counterpart of the .xs file.

Oh, I see. Not sure. For myself, I only care about files that survive
"make distclean" and get into a tarball, which those don't. On the
other hand, if we fixed perlchunks.h and plperl_opmask.h then it's hard
to argue with worrying about SPI.c and Util.c, as those all have the
same lifespan.

I tried redoing the experiment of pgindenting all the tarball member
files, and found that we still have diffs in these generated files:

src/backend/utils/sort/qsort_tuple.c
src/common/kwlist_d.h
src/interfaces/ecpg/preproc/c_kwlist_d.h
src/interfaces/ecpg/preproc/ecpg_kwlist_d.h
src/pl/plpgsql/src/pl_reserved_kwlist_d.h
src/pl/plpgsql/src/pl_unreserved_kwlist_d.h
src/pl/plpgsql/src/plerrcodes.h
src/pl/plpython/spiexceptions.h
src/pl/tcl/pltclerrcodes.h

To my eyes, what pgindent does to the *kwlist_d.h files is rather ugly,
so I'm inclined to make them exclusions rather than adjust the generator
script. The others seem like we could tweak the relevant generators
fairly easily.

regards, tom lane

I went ahead and added SPI.c, Util.c, and the *kwlist_d.h headers
to the exclusion list. I then tried to run pgindent in a completely
built-out development directory (not distclean'ed, which is the way
I'd always used it before). This found a few more exclusions we
need to have if we want to allow for that usage. Pushed the lot.

We still have to deal with

src/backend/utils/sort/qsort_tuple.c
src/pl/plpgsql/src/plerrcodes.h
src/pl/plpython/spiexceptions.h
src/pl/tcl/pltclerrcodes.h

if we want to be entirely clean about this.

regards, tom lane

I wrote:

We still have to deal with
src/backend/utils/sort/qsort_tuple.c
src/pl/plpgsql/src/plerrcodes.h
src/pl/plpython/spiexceptions.h
src/pl/tcl/pltclerrcodes.h
if we want to be entirely clean about this.

I took care of those, so I think we're done here.

regards, tom lane

On 21 Sep 2020, at 19:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

We still have to deal with
src/backend/utils/sort/qsort_tuple.c
src/pl/plpgsql/src/plerrcodes.h
src/pl/plpython/spiexceptions.h
src/pl/tcl/pltclerrcodes.h
if we want to be entirely clean about this.

I took care of those, so I think we're done here.

Aha, thanks! They were still on my TODO but I got distracted by a shiny object
or two. Thanks for fixing.

cheers ./daniel

Oh wait, I forgot about the fmgrprotos.h discrepancy.

I wrote:

It strikes me that a low-cost workaround would be to rename these
C functions. There's no law that their C names must match the
SQL names.

Here's a proposed patch to fix it that way.

regards, tom lane

On 2020-Sep-21, Tom Lane wrote:

Oh wait, I forgot about the fmgrprotos.h discrepancy.

I wrote:

It strikes me that a low-cost workaround would be to rename these
C functions. There's no law that their C names must match the
SQL names.

Here's a proposed patch to fix it that way.

pgtypes_numeric.h still contains

typedef struct
{
int ndigits; /* number of digits in digits[] - can be 0! */
int weight; /* weight of first digit */
int rscale; /* result scale */
int dscale; /* display scale */
int sign; /* NUMERIC_POS, NUMERIC_NEG, or NUMERIC_NAN */
NumericDigit *buf; /* start of alloc'd space for digits[] */
NumericDigit *digits; /* decimal digits */
} numeric;

... isn't this more likely to create a typedef entry than merely a
function name?

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2020-Sep-21, Tom Lane wrote:

Here's a proposed patch to fix it that way.

pgtypes_numeric.h still contains
typedef struct
{
} numeric;

... isn't this more likely to create a typedef entry than merely a
function name?

Well, yeah, it *is* a typedef. My proposal is to rename the C function
to avoid the conflict, rather than renaming the typedef. Given the
small number of direct calls (none), that's a lot less work. Also,
I think pgtypes_numeric.h is exposed to ecpg client code, so changing
that typedef's name could be quite problematic.

regards, tom lane

On 2020-Sep-21, Tom Lane wrote:

... isn't this more likely to create a typedef entry than merely a
function name?

Well, yeah, it *is* a typedef. My proposal is to rename the C function
to avoid the conflict, rather than renaming the typedef. Given the
small number of direct calls (none), that's a lot less work. Also,
I think pgtypes_numeric.h is exposed to ecpg client code, so changing
that typedef's name could be quite problematic.

Ah, of course.

The idea of adding the names to pgindent's %blacklist results in severe
uglification, particularly in the regex code, so +1 for your workaround.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 21 Sep 2020, at 20:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Oh wait, I forgot about the fmgrprotos.h discrepancy.

I wrote:

It strikes me that a low-cost workaround would be to rename these
C functions. There's no law that their C names must match the
SQL names.

Here's a proposed patch to fix it that way.

+1 on this patch. Do you think it's worth adding a note about this in the
documentation to save the next one staring at this a few minutes? Something
along the lines of:

--- a/src/tools/pgindent/README
+++ b/src/tools/pgindent/README
@@ -110,6 +110,9 @@ Sometimes, if pgindent or perltidy produces odd-looking output, it's because
 of minor bugs like extra commas.  Don't hesitate to clean that up while
 you're at it.

+If an exported function shares a name with a typedef, the header file with the
+prototype can get incorrect spacing for the function.
+

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:

On 21 Sep 2020, at 20:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Here's a proposed patch to fix it that way.

+1 on this patch. Do you think it's worth adding a note about this in the
documentation to save the next one staring at this a few minutes? Something
along the lines of:

+If an exported function shares a name with a typedef, the header file with the
+prototype can get incorrect spacing for the function.

AFAIK, a function name that's the same as a typedef name will get
misformatted whether it's exported or not; pgindent doesn't really
know the difference. But yeah, we could add something about this.

regards, tom lane

After further thought about this, I concluded that a much better idea
is to just exclude fmgrprotos.h from the pgindent run. While renaming
these two functions may or may not be worthwhile, it doesn't provide
any sort of permanent fix for fmgrprotos.h, because new typedef conflicts
could arise at any time. (The fact that the typedef list isn't fully
under our control, thanks to contributions from system headers, makes
this a bigger risk than it might appear.)

So I did that, and also added a README comment along the lines you
suggested.

regards, tom lane