More const-marking cleanup

On Fri, Dec 5, 2025 at 11:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Apparently, latest gcc is able to notice that constructions like

const char *str = ...;
char *ptr = strchr(str, ':');

are effectively casting away const. This is a good thing and long
overdue, but we have some work to do to clean up the places where
we are doing that.

Yeah, one of the qualifier-preserving generic functions that C23
invented: bsearch, memchr, strchr, strpbrk, strrchr, strstr, wcschr,
wcspbrk, wcsrchr, wmemchr, and wcsstr. The synopses use QVoid or
QChar to mean "same qualifier", a bit like C++ function templates. We
could probably benefit from some of that in our own code node, list,
tree etc code, as it only requires C11 _Generic to implement.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf

Hi,

On Thu, Dec 04, 2025 at 05:09:30PM -0500, Tom Lane wrote:

Attached is a patch that cleans up all the cases I saw on a local
rawhide installation.

What about adding the 4 in the attached?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:

What about adding the 4 in the attached?

Cool, how'd you find these?

regards, tom lane

Hi,

On Fri, Dec 05, 2025 at 09:47:56AM -0500, Tom Lane wrote:

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:

What about adding the 4 in the attached?

Cool, how'd you find these?

With the help of a coccinelle script. Will polish and share.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:

On Fri, Dec 05, 2025 at 09:47:56AM -0500, Tom Lane wrote:

Cool, how'd you find these?

With the help of a coccinelle script. Will polish and share.

Ah. Your script didn't notice the need for const'ification of
additional variables in map_locale() :-(. I fixed that and
pushed everything except the ecpg/preproc/variable.c changes,
which I'm not too comfortable about yet. (The code coverage
report shows that large chunks of those functions are untested,
so I wonder if they worked before let alone after.)

regards, tom lane

I wrote:

Ah. Your script didn't notice the need for const'ification of
additional variables in map_locale() :-(. I fixed that and
pushed everything except the ecpg/preproc/variable.c changes,
which I'm not too comfortable about yet. (The code coverage
report shows that large chunks of those functions are untested,
so I wonder if they worked before let alone after.)

I was right to be suspicious about variable.c: I'd misunderstood
what was happening in find_struct_member(), with the consequence
that I broke some cases that were not being tested. Here's v2,
with that repaired, more commentary, and more test cases.

Adding these comments feels a bit like putting lipstick on a pig.
find_variable and its subroutines are an inelegant, duplicative
mess that fails to handle cases it easily could handle if it were
rewritten. But I've put enough brain cells into this already,
and also it appears that there are restrictions elsewhere in ecpg
that'd have to be lifted before it'd make a difference.

regards, tom lane

Hi,

On Fri, Dec 05, 2025 at 03:52:40PM -0500, Tom Lane wrote:

I wrote:

Ah. Your script didn't notice the need for const'ification of
additional variables in map_locale() :-(.

Ah, right, thanks for letting me know. It would need more work to catch those.

That said I improved the script [1]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/indirectly_casting_away_const.cocci so that:

- It found one more (see the attached)
- It is able to detect cases even for functions created in the code tree. It found
that skip_drive() (path.c) and bsearch_arg() (bsearch_arg.c) are also functions
that cast away const in their return values
- It also checked callers for the 2 functions above and did not find const to add

Adding these comments feels a bit like putting lipstick on a pig.
find_variable and its subroutines are an inelegant, duplicative
mess that fails to handle cases it easily could handle if it were
rewritten. But I've put enough brain cells into this already,
and also it appears that there are restrictions elsewhere in ecpg
that'd have to be lifted before it'd make a difference.

I also look at those (even if already pushed in 4eda42e8bdf) and they LGTM.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/indirectly_casting_away_const.cocci

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Mon, Dec 08, 2025 at 07:43:34AM +0000, Bertrand Drouvot wrote:

That said I improved the script [1] so that:

- It found one more (see the attached)

I had another look and it seems to me that the one (src/port/getopt.c) reported
and fixed in the attached of the previous email is the only remaining one to fix.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:

I had another look and it seems to me that the one (src/port/getopt.c) reported
and fixed in the attached of the previous email is the only remaining one to fix.

This fell off my radar for a bit, but pushed now. Thanks for
catching it.

This does raise a question in my mind though: we did not find this
case the first time around because machines that have new-enough
compilers to detect such issues will probably not need our version of
getopt(). So, what other not-always-compiled bits of code could use
improvement, and how could we find them without undue effort? (This
extends to more issues than just casting-away-const of course.)

regards, tom lane

Hi,

On Tue, Dec 23, 2025 at 09:47:55PM -0500, Tom Lane wrote:

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:

I had another look and it seems to me that the one (src/port/getopt.c) reported
and fixed in the attached of the previous email is the only remaining one to fix.

This fell off my radar for a bit, but pushed now.

Thanks!

This does raise a question in my mind though: we did not find this
case the first time around because machines that have new-enough
compilers to detect such issues will probably not need our version of
getopt(). So, what other not-always-compiled bits of code could use
improvement, and how could we find them without undue effort? (This
extends to more issues than just casting-away-const of course.)

That's a fair point. In fact this particular one has been found with the help of
a Coccinelle script, so your remark makes me think of this thread [1]/messages/by-id/aQh79jMqU7mq03Hv@ip-10-97-1-34.eu-west-3.compute.internal. Indeed,
Coccinelle could help address this by analyzing the code tree without needing to
compile it.

We could imagine:

- adding Coccinelle scripts to the code tree, or
- running BF members with a set of Coccinelle scripts, or
- running a set of Coccinelle scripts at regular intervals

Analyzing the code tree without the need to compile it looks like a valid answer
to your concern, thoughts?

[1]: /messages/by-id/aQh79jMqU7mq03Hv@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com