Обсуждение: Mark function arguments of type "Datum *" as "const Datum *" where possible

On Fri, Sep 26, 2025 at 03:35:54PM +0800, Chao Li wrote:
>> I noticed that many functions take "Datum *" parameters while they don't
>> update the data. So I created this patch to change "Datum *" to "const
>> Datum *" wherever possible, which should improve type safety and make the
>> interfaces clearer about their intent, also helps the compiler catch
>> accidental modifications.

I have not looked at the patch in details and we need a careful
case-by-case review, but being more protective with the Datums that
travel across the stack may be a good idea in the long-term depending
on the code path dealt with, so I like the initiative you are taking
here.
--
Michael

On Fri, Sep 26, 2025 at 08:50:10PM +0900, Michael Paquier wrote:
> On Fri, Sep 26, 2025 at 03:35:54PM +0800, Chao Li wrote:
>>> I noticed that many functions take "Datum *" parameters while they don't
>>> update the data. So I created this patch to change "Datum *" to "const
>>> Datum *" wherever possible, which should improve type safety and make the
>>> interfaces clearer about their intent, also helps the compiler catch
>>> accidental modifications.
> 
> I have not looked at the patch in details and we need a careful
> case-by-case review, but being more protective with the Datums that
> travel across the stack may be a good idea in the long-term depending
> on the code path dealt with, so I like the initiative you are taking
> here.

I have mixed feelings about this patch.  I have no concrete objections to
the technical content, but some questions come to mind.  For example, why
are we only fixing Datum parameters?  Are we going to fix other types
later?  Also, I'm a bit worried that this will lead to several rounds of
similar patches, which IMHO is arguably unnecessary churn that could lead
to back-patching pain.  I personally would be more likely to fix these
sorts of things in passing as part of another patch that touches the same
area of code.

That being said, I won't object if Michael or someone else feels it's
worthwhile.  I usually try to add const to new function parameters whenever
possible, for exactly the reasons given as motivation for this patch.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> I have mixed feelings about this patch.  I have no concrete objections to
> the technical content, but some questions come to mind.  For example, why
> are we only fixing Datum parameters?

Yeah.  In particular, probably 99% of such Datum arrays also have an
associated array of bool isnull flags.  IMO it makes exactly zero
sense to const-ify the Datums without similar protection for their
isnull flags.

            regards, tom lane