Re: [HACKERS] confusing typedefs in jsonfuncs.c

2013-07-20 Thread Peter Eisentraut 
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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] confusing typedefs in jsonfuncs.c

2013-07-19 Thread Andrew Dunstan 


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

GetStatestate;

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] confusing typedefs in jsonfuncs.c

2013-07-18 Thread Peter Eisentraut 
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

GetStatestate;

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.

diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index a231736..ecfe063 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -51,11 +51,11 @@
 static inline void json_lex(JsonLexContext *lex);
 static inline void json_lex_string(JsonLexContext *lex);
 static inline void json_lex_number(JsonLexContext *lex, char *s);
-static inline void parse_scalar(JsonLexContext *lex, JsonSemAction sem);
-static void parse_object_field(JsonLexContext *lex, JsonSemAction sem);
-static void parse_object(JsonLexContext *lex, JsonSemAction sem);
-static void parse_array_element(JsonLexContext *lex, JsonSemAction sem);
-static void parse_array(JsonLexContext *lex, JsonSemAction sem);
+static inline void parse_scalar(JsonLexContext *lex, JsonSemAction *sem);
+static void parse_object_field(JsonLexContext *lex, JsonSemAction *sem);
+static void parse_object(JsonLexContext *lex, JsonSemAction *sem);
+static void parse_array_element(JsonLexContext *lex, JsonSemAction *sem);
+static void parse_array(JsonLexContext *lex, JsonSemAction *sem);
 static void report_parse_error(JsonParseContext ctx, JsonLexContext *lex);
 static void report_invalid_token(JsonLexContext *lex);
 static int	report_json_context(JsonLexContext *lex);
@@ -70,12 +70,11 @@ static void array_to_json_internal(Datum array, StringInfo result,
 	   bool use_line_feeds);
 
 /* the null action object used for pure validation */
-static jsonSemAction nullSemAction =
+static JsonSemAction nullSemAction =
 {
 	NULL, NULL, NULL, NULL, NULL,
 	NULL, NULL, NULL, NULL, NULL
 };
-static JsonSemAction NullSemAction = nullSemAction;
 
 /* Recursive Descent parser support routines */
 
@@ -170,7 +169,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 
 	/* validate it */
 	lex = makeJsonLexContext(result, false);
-	pg_parse_json(lex, NullSemAction);
+	pg_parse_json(lex, nullSemAction);
 
 	/* Internal representation is the same as text, for now */
 	PG_RETURN_TEXT_P(result);
@@ -222,7 +221,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 
 	/* Validate it. */
 	lex = makeJsonLexContext(result, false);
-	pg_parse_json(lex, NullSemAction);
+	pg_parse_json(lex, nullSemAction);
 
 	PG_RETURN_TEXT_P(result);
 }
@@ -260,7 +259,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
  * pointer to a state object to be passed to those routines.
  */
 void
-pg_parse_json(JsonLexContext *lex, JsonSemAction sem)
+pg_parse_json(JsonLexContext *lex, JsonSemAction *sem)
 {
 	JsonTokenType tok;
 
@@ -296,7 +295,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
  *	  - object field
  */
 static inline void
-parse_scalar(JsonLexContext *lex, JsonSemAction sem)
+parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
 {
 	char	   *val = NULL;
 	json_scalar_action sfunc = sem-scalar;
@@ -332,7 +331,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 }
 
 static void
-parse_object_field(JsonLexContext *lex, JsonSemAction sem)
+parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
 {
 	/*
 	 * an object field is fieldname : value where value can be a scalar,
@@ -380,7 +379,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 }
 
 static void
-parse_object(JsonLexContext *lex, JsonSemAction sem)
+parse_object(JsonLexContext *lex, JsonSemAction *sem)
 {
 	/*
 	 * an object is a possibly empty sequence of object fields, separated by
@@ -428,7 +427,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 }
 
 static void
-parse_array_element(JsonLexContext *lex, JsonSemAction sem)
+parse_array_element(JsonLexContext *lex, JsonSemAction *sem)
 {
 	json_aelem_action astart = sem-array_element_start;
 	json_aelem_action aend = sem-array_element_end;
@@ -459,7 +458,7 @@ static void array_to_json_internal(Datum array, StringInfo result,
 }
 
 static void
-parse_array(JsonLexContext *lex, JsonSemAction sem)
+parse_array(JsonLexContext *lex, JsonSemAction *sem)
 {

Re: [HACKERS] confusing typedefs in jsonfuncs.c

2013-07-18 Thread Tom Lane 
Peter Eisentraut pete...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers