On Mon, Jan 27, 2020 at 3:05 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I’m attaching a new patch set with these three changes including Mahendra’s patch posted elsewhere on this thread.
>
> Since you’ve committed your 0004 and 0005 patches, this v6 patch set is now based on a fresh copy of master.
OK, so I think this is getting close.
What is now 0001 manages to have four (4) conditionals on FRONTEND at
the top of the file. This seems like at least one two many. I am OK
with this being separate:
+#ifndef FRONTEND
#include "postgres.h"
+#else
+#include "postgres_fe.h"
+#endif
postgres(_fe).h has pride of place among includes, so it's reasonable
to put this in its own section like this.
+#ifdef FRONTEND
+#define check_stack_depth()
+#define json_log_and_abort(...) pg_log_fatal(__VA_ARGS__); exit(1);
+#else
+#define json_log_and_abort(...) elog(ERROR, __VA_ARGS__)
+#endif
OK, so here we have a section entirely devoted to our own file-local
macros. Also reasonable. But in between, you have both an #ifdef
FRONTEND and an #ifndef FRONTEND for other includes, and I really
think that should be like #ifdef FRONTEND .. #else .. #endif.
Also, the preprocessor police are on their way to your house now to
arrest you for that first one. You need to write it like this:
#define json_log_and_abort(...) \
do { pg_log_fatal(__VA_ARGS__); exit(1); } while (0)
Otherwise, hilarity ensues if somebody writes if (my_code_is_buggy)
json_log_and_abort("oops").
{
- JsonLexContext *lex = palloc0(sizeof(JsonLexContext));
+ JsonLexContext *lex;
+
+#ifndef FRONTEND
+ lex = palloc0(sizeof(JsonLexContext));
+#else
+ lex = (JsonLexContext*) malloc(sizeof(JsonLexContext));
+ memset(lex, 0, sizeof(JsonLexContext));
+#endif
Instead of this, how making no change at all here?
- default:
- elog(ERROR, "unexpected json parse state: %d", ctx);
}
+
+ /* Not reached */
+ json_log_and_abort("unexpected json parse state: %d", ctx);
This, too, seems unnecessary.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
