Re: ECPG cleanup and fix for clang compile-time problem

From: John Naylor <johncnaylorls(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Peter Eisentraut <peter(at)eisentraut(dot)org>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Subject: Re: ECPG cleanup and fix for clang compile-time problem
Date: 2024-10-11 11:43:22
Message-ID: CANWCAZYNiRUZ7nOMNhMXJ9reO8SBGawx74519jZXNa7nrvr0qA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> [v5]

0001 - LGTM, maybe can be squashed with 0009?

0002 - I went through this again and don't see anything that should
raise eyebrows.

+ # HACK: insert our own %nonassoc line after IDENT.
+ # XXX: this seems pretty wrong, IDENT is not last on its line!

We can come back to this afterwards, as mentioned elsewhere in the thread.

0003

Clang is what motivated this, but gcc also shows a speedup -- from
5.9s to 1.6s on this old machine, which is great. The giant switch
statement in preproc.c has about 1/10 the labels as before.

+In the original implementation of ecpg, the strings constructed
+by grammar rules were returned as the Bison result of each rule.
+This led to a large number of effectively-identical rule actions,
+which caused compilation-time problems with some versions of clang.
+Now, rules that need to return a string are declared as having

"Original" is going to be a mystery in a few years -- I'd describe
this in terms of "as of PG18" or some such.

+ * is producing uniformly-cased output of keywords. (That's mostly
+ * cosmetic, but there are places in ecpglib that expect to receive
+ * downcased keywords, plus it keeps us regression-test-compatible
+ * with the old implementation of ecpg.)

Ditto with "old".

+ /* List a token here if pgc.l assigns to base_yylval.str for it */

Does pgc.l need to have a similar comment?

0004 seems like a sensible reorg.

0005 - I wondered if the change of YYLTYPE to "const char *" can be
done in a separate commit to make the other changes more legible, but
that might not be worth the effort.

+ * "Local" (or "location"?) memory management support

"Local" seems to fit well enough. Tying the arena to the statement
level seems sound.

I haven't looked closely at 0006 through 0009. One possible concern is
that the regression tests might not cover very well, but if you can
get valgrind silent for memory leaks for what they do cover, that's
certainly a good step.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Chapman Flack 2024-10-11 12:42:19 Re: Doc of typmod arg perhaps deserves an update
Previous Message David Rowley 2024-10-11 09:23:17 Re: gcc-14.1.0 [-Werror=array-bounds=] meson buildtype=release compile error