Обсуждение: 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.