Thread: pgindent vs dtrace on macos
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
Attachment
> 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
Attachment
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
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 27d65557df..2cc2da60eb 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -1109,7 +1109,7 @@ numeric_support(PG_FUNCTION_ARGS)
* scale of the attribute have to be applied on the value.
*/
Datum
-numeric (PG_FUNCTION_ARGS)
+numeric_apply_typmod(PG_FUNCTION_ARGS)
{
Numeric num = PG_GETARG_NUMERIC(0);
int32 typmod = PG_GETARG_INT32(1);
diff --git a/src/backend/utils/adt/oracle_compat.c b/src/backend/utils/adt/oracle_compat.c
index 76e666474e..79ba28671f 100644
--- a/src/backend/utils/adt/oracle_compat.c
+++ b/src/backend/utils/adt/oracle_compat.c
@@ -923,7 +923,7 @@ ascii(PG_FUNCTION_ARGS)
********************************************************************/
Datum
-chr (PG_FUNCTION_ARGS)
+chr_code_to_text(PG_FUNCTION_ARGS)
{
uint32 cvalue = PG_GETARG_UINT32(0);
text *result;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f48f5fb4d9..5eb293ee8f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3399,7 +3399,7 @@
prosrc => 'ascii' },
{ oid => '1621', descr => 'convert int4 to char',
proname => 'chr', prorettype => 'text', proargtypes => 'int4',
- prosrc => 'chr' },
+ prosrc => 'chr_code_to_text' },
{ oid => '1622', descr => 'replicate string n times',
proname => 'repeat', prorettype => 'text', proargtypes => 'text int4',
prosrc => 'repeat' },
@@ -4210,7 +4210,8 @@
proargtypes => 'internal', prosrc => 'numeric_support' },
{ oid => '1703', descr => 'adjust numeric to typmod precision/scale',
proname => 'numeric', prosupport => 'numeric_support',
- prorettype => 'numeric', proargtypes => 'numeric int4', prosrc => 'numeric' },
+ prorettype => 'numeric', proargtypes => 'numeric int4',
+ prosrc => 'numeric_apply_typmod' },
{ oid => '1704',
proname => 'numeric_abs', prorettype => 'numeric', proargtypes => 'numeric',
prosrc => 'numeric_abs' },
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