Horton

Thread

Commits

  1. Exclude fmgrprotos.h from pgindent processing.

  2. Fix a few more generator scripts to produce pgindent-clean output.

  3. Further improve pgindent's list of file exclusions.

  4. Improve formatting of create_help.pl and plperl_opmask.pl output.

  1. pgindent vs dtrace on macos

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-20T09:52:25Z

    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?

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

  2. Re: pgindent vs dtrace on macos

    Daniel Gustafsson <daniel@yesql.se> — 2020-05-20T10:01:23Z

    > 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

  3. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-20T13:56:55Z

    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

  4. Re: pgindent vs dtrace on macos

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-22T09:29:25Z

    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

  5. Re: pgindent vs dtrace on macos

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-16T21:17:46Z

    > 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

  6. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-16T23:55:17Z

    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

  7. Re: pgindent vs dtrace on macos

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-17T00:03:34Z

    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

  8. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T00:34:55Z

    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

  9. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T00:39:52Z

    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

  10. Re: pgindent vs dtrace on macos

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-17T08:15:30Z

    >>> 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

  11. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T15:39:56Z

    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

  12. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T18:20:28Z

    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

  13. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-21T17:59:20Z

    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

  14. Re: pgindent vs dtrace on macos

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-21T18:14:03Z

    > 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

  15. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-21T18:55:37Z

    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

  16. Re: pgindent vs dtrace on macos

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-21T19:08:04Z

    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

  17. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-21T19:21:12Z

    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

  18. Re: pgindent vs dtrace on macos

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-21T19:37:34Z

    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

  19. Re: pgindent vs dtrace on macos

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-22T08:25:54Z

    > 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

  20. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-22T14:18:40Z

    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

  21. Re: pgindent vs dtrace on macos

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-22T15:35:55Z

    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