Peter Eisentraut <peter.eisentr...@enterprisedb.com> writes:
> On 17.05.22 20:43, Tom Lane wrote:
>> So I think Peter's got a good idea here (I might quibble with the details
>> of some of these macros). But it's not really going to move the
>> safety goalposts very far unless we make a concerted effort to make
>> these be the style everywhere. Are we willing to do that? What
>> will it do to back-patching difficulty? Dare I suggest back-patching
>> these changes?
> I think it could go like the castNode() introduction: first we adopt it
> sporadically for new code, then we change over some larger pieces of
> code, then we backpatch the API, then someone sends in a big patch to
> change the rest.
OK, that seems like a reasonable plan.
I've now studied this a little more closely, and I think the
functionality is fine, but I have naming quibbles.
1. Do we really want distinct names for the frontend and backend
versions of the macros? Particularly since you're layering the
frontend ones over pg_malloc, which has exit-on-OOM behavior?
I think we've found that notational discrepancies between frontend
and backend code are generally more a hindrance than a help,
so I'm inclined to drop the pg_malloc_xxx macros and just use
"palloc"-based names across the board.
2. I don't like the "palloc_ptrtype" name at all. I see that you
borrowed that name from talloc, but I doubt that's a precedent that
very many people are familiar with. To me it sounds like it might
allocate something that's the size of a pointer, not the size of the
pointed-to object. I have to confess though that I don't have an
obviously better name to suggest. "palloc_pointed_to" would be
clear perhaps, but it's kind of long.
3. Likewise, "palloc_obj" is perhaps less clear than it could be.
I find palloc_array just fine though. Maybe palloc_object or
palloc_struct? (If "_obj" can be traced to talloc, I'm not
seeing where.)
One thought that comes to mind is that palloc_ptrtype is almost
surely going to be used in the style
myvariable = palloc_ptrtype(myvariable);
and if it's not that it's very likely wrong. So maybe we should cut
out the middleman and write something like
#define palloc_instantiate(ptr) ((ptr) = (typeof(ptr)) palloc(sizeof(*(ptr))))
...
palloc_instantiate(myvariable);
I'm not wedded to "instantiate" here, there's probably better names.
regards, tom lane
