From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
cc: Peter Eisentraut <peter@eisentraut.org>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Remove useless casts to (void *)
In-reply-to: <CA+hUKG+NFKnr=K4oybwDvT35dW=VAjAAfiuLxp+5JeZSOV3nBg@mail.gmail.com>
References: <461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org> <CA+hUKG+NFKnr=K4oybwDvT35dW=VAjAAfiuLxp+5JeZSOV3nBg@mail.gmail.com>
Comments: In-reply-to Thomas Munro <thomas.munro@gmail.com>
	message dated "Tue, 03 Dec 2024 12:21:33 +1300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1709837.1733189773.1@sss.pgh.pa.us>
Date: Mon, 02 Dec 2024 20:36:13 -0500
Message-ID: <1709838.1733189773@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1709838.1733189773%40sss.pgh.pa.us>
Precedence: bulk

Thomas Munro <thomas.munro@gmail.com> writes:
> Why do we bother with a "Pointer" type?  The idiomatic way to do
> byte-oriented pointer arithmetic is to cast to char *, or uint8_t*
> etc, which doesn't require the reader to go and figure out what stride
> that non-standard type name implies.

I think getting rid of Pointer altogether would cause a lot of code
churn for not that much benefit; most places are (I think) just using
it as a name for a generic pointer.  I could support a patch to
define it as "void *" not "char *", if there aren't too many places
that would have to be modified.

> DatumGetPointer() should arguably return void *
> too, to force users to provide a type if they're going to dereference
> or perform arithmetic.

Well, it returns Pointer, which is what it should return.

> What problem does PointerIsValid(x) solve, when you could literally
> just write x or if you insist x != NULL in most contexts and it would
> be 100% idiomatic C, and shorter?

The same goes for a number of other historical macros, such as
OidIsValid.  I think there was possibly once an idea that super-duper
debug builds could apply stricter tests in these macros.  Nothing's
ever been done in that line, but that doesn't make it an awful idea.
I don't particularly object to code that just checks "if (x)", but
I wouldn't be in favor of removing these macros, if only because the
sheer size of the patch would make it a back-patching land mine.

> Why do all the XLogRegister*() calls have to cast their argument to
> char *?  Seems like a textbook use for void * argument, not requiring
> a cast.

Probably.  Again, it'd be interesting to try changing it and see how
invasive the patch winds up being.

> (While grepping for casts to char *, I had a mistake in my regex and
> finished up seeing how many places in our code check sizeof(char),
> which is funny because sizeof is defined as telling you how many chars
> it takes to hold a type/value; perhaps it has documentation value :-))

Yeah.  I think that "ptr = palloc(n * sizeof(char))" is good practice
as documentation, even when we all know "sizeof(char)" is 1.

			regards, tom lane