Обсуждение: confusing typedefs in jsonfuncs.c

The new jsonfuncs.c has some confusing typedef scheme.  For example, it
has a bunch of definitions like this:

typedef struct getState
{
    ...
} getState, *GetState;

So GetState is a pointer to getState.  I have never seen that kind of
convention before.

This then leads to code like

GetState    state;

state = palloc0(sizeof(getState));

which has useless mental overhead.

But the fact that GetState is really a pointer isn't hidden at all,
because state is then derefenced with -> or cast from or to void*.  So
whatever abstraction might have been intended isn't there at all.  And
all of this is an intra-file interface anyway.

And to make this even more confusing, other types such as ColumnIOData
and JsonLexContext are not pointers but structs directly.

I think a more typical PostgreSQL code convention is to use capitalized
camelcase for structs, and use explicit pointers for pointers.  I have
attached a patch that cleans this up, in my opinion.

Peter Eisentraut <peter_e@gmx.net> writes:
> The new jsonfuncs.c has some confusing typedef scheme.  For example, it
> has a bunch of definitions like this:

> typedef struct getState
> {
>     ...
> } getState, *GetState;

> So GetState is a pointer to getState.  I have never seen that kind of
> convention before.

Yeah, this is randomly different from everywhere else in PG.  The more
usual convention if you want typedefs for both the struct and the
pointer type is that the pointer type is FooBar and the struct type is
FooBarData.  This way seems seriously typo-prone.

> I think a more typical PostgreSQL code convention is to use capitalized
> camelcase for structs, and use explicit pointers for pointers.  I have
> attached a patch that cleans this up, in my opinion.

That way is fine with me too.

If you commit this, please hit 9.3 as well, so that we don't have
back-patching issues.
        regards, tom lane

On 07/18/2013 09:20 PM, Peter Eisentraut wrote:
> The new jsonfuncs.c has some confusing typedef scheme.  For example, it
> has a bunch of definitions like this:
>
> typedef struct getState
> {
>      ...
> } getState, *GetState;
>
> So GetState is a pointer to getState.  I have never seen that kind of
> convention before.
>
> This then leads to code like
>
> GetState    state;
>
> state = palloc0(sizeof(getState));
>
> which has useless mental overhead.
>
> But the fact that GetState is really a pointer isn't hidden at all,
> because state is then derefenced with -> or cast from or to void*.  So
> whatever abstraction might have been intended isn't there at all.  And
> all of this is an intra-file interface anyway.
>
> And to make this even more confusing, other types such as ColumnIOData
> and JsonLexContext are not pointers but structs directly.
>
> I think a more typical PostgreSQL code convention is to use capitalized
> camelcase for structs, and use explicit pointers for pointers.  I have
> attached a patch that cleans this up, in my opinion.


I don't have a problem with this. Sometimes when you've stared at 
something for long enough you fail to notice such things, so I welcome 
more eyeballs on the code.

Note that this is an externally visible API, so anyone who has written 
an extension against it will possibly find it broken. But that's all the 
more reason to clean it now, before it makes it to a released version.

cheers

andrew

On Thu, 2013-07-18 at 21:34 -0400, Tom Lane wrote:
> Yeah, this is randomly different from everywhere else in PG.  The more
> usual convention if you want typedefs for both the struct and the
> pointer type is that the pointer type is FooBar and the struct type is
> FooBarData.

I think that is more useful if you have a really good abstraction around
FooBar, like for Relation, for example.  That's not really the case
here.  This is more like node types, perhaps.