В письме от воскресенье, 24 февраля 2019 г. 14:31:55 MSK пользователь Dmitry
Belyavsky написал:
Hi! Am back here again.
I've been thinking about this patch a while... Come to some conclusions and
wrote some examples...
First I came to idea that the best way to simplify the code is change the
state machine from if to switch/case. Because in your code a lot of repetition
is done just because you can't say "Thats all, let's go to next symbol" in any
place in the code. Now it should follow all the branches of if-else tree that
is inside the state-machine "node" to get to the next symbol.
To show how simpler the things would be I changed the state machine processing
in lquery_in form if to switch/case, and changed the code for LQPRS_WAITLEVEL
state processing, removing duplicate code, using "break" wherever it is
possible
(The indention in this example is unproper to make diff more clear)
so from that much of code
=================
if (state == LQPRS_WAITLEVEL)
{
if (t_isspace(ptr))
{
ptr += charlen;
pos++;
continue;
}
escaped_count = 0;
real_levels++;
if (charlen == 1)
{
if (t_iseq(ptr, '!'))
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr + 1;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
curqlevel->flag |= LQL_NOT;
hasnot = true;
}
else if (t_iseq(ptr, '*'))
state = LQPRS_WAITOPEN;
else if (t_iseq(ptr, '\\'))
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr;
curqlevel->numvar = 1;
state = LQPRS_WAITESCAPED;
}
else if (strchr(".|@%{}", *ptr))
{
UNCHAR;
}
else
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
if (t_iseq(ptr, '"'))
{
lptr->flag |= LVAR_QUOTEDPART;
}
}
}
else
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) *
numOR);
lptr->start = ptr;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
}
}
=====================
I came to this
=====================
case LQPRS_WAITLEVEL:
if (t_isspace(ptr))
break; /* Just go to next symbol */
escaped_count = 0;
real_levels++;
if (charlen == 1)
{
if (strchr(".|@%{}", *ptr))
UNCHAR;
if (t_iseq(ptr, '*'))
{
state = LQPRS_WAITOPEN;
break;
}
}
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) *
numOR);
lptr->start = ptr;
curqlevel->numvar = 1;
state = LQPRS_WAITDELIM;
if (charlen == 1)
{
if (t_iseq(ptr, '\\'))
{
state = LQPRS_WAITESCAPED;
break;
}
if (t_iseq(ptr, '!'))
{
lptr->start += 1 /*FIXME explain why */;
curqlevel->flag |= LQL_NOT;
hasnot = true;
}
else if (t_iseq(ptr, '"'))
{
lptr->flag |= LVAR_QUOTEDPART;
}
}
break;
=======
And this code is much more readable.... You can just read it aloud:
-Spaces we just skip
- on .|@%{} throws and exception
- for asterisk change state and nothing else
then for others allocate a buffer
- for slash we just set a flag
- for exclamation mark we set a flag and skip it
- for double quote set a flag.
All the logic are clear. And in your version of the code it is difficult to get
the idea from the code that simple.
So my suggestion is following:
1. Submit and commit the patch that changes state machines from ifs to switch/
cases, and nothing else. This will reindent the code, so in order to keep
further changes visible, we should change nothing else.
2. Change your code to switch/cases, use break to deduplicate code wherever is
possible, and commit it.
I can join you while working with this (but then I am not sure I would be able
to remain a reviewer...)
I've also attached an example I've quoted above, formatted as a diff, so it
would be easy to check how does it work. It should be applied after your
patch.
(I gave it a strange name ltree.not-a-patch hoping that commitfest software
will not treat it a new version of a patch)diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,6 +31,7 @@ win32ver.rc
*.exe
lib*dll.def
lib*.pc
+tags
# Local excludes in root directory
/GNUmakefile
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index e086c09..0ffb4b8 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -521,64 +521,50 @@ lquery_in(PG_FUNCTION_ARGS)
while (*ptr)
{
charlen = pg_mblen(ptr);
-
- if (state == LQPRS_WAITLEVEL)
+ switch (state)
{
+ case LQPRS_WAITLEVEL:
if (t_isspace(ptr))
- {
- ptr += charlen;
- pos++;
- continue;
- }
+ break; /* Just go to next symbol */
escaped_count = 0;
real_levels++;
+
if (charlen == 1)
{
- if (t_iseq(ptr, '!'))
+ if (strchr(".|@%{}", *ptr))
+ UNCHAR;
+
+ if (t_iseq(ptr, '*'))
{
- GETVAR(curqlevel) = lptr = (nodeitem *) palloc0(sizeof(nodeitem) * numOR);
- lptr->start = ptr + 1;
- state = LQPRS_WAITDELIM;
- curqlevel->numvar = 1;
- curqlevel->flag |= LQL_NOT;
- hasnot = true;
- }
- else if (t_iseq(ptr, '*'))
state = LQPRS_WAITOPEN;
- else if (t_iseq(ptr, '\\'))
+ break;
+ }
+ }
+ GETVAR(curqlevel) = lptr = (nodeitem *) palloc0(sizeof(nodeitem) * numOR);
+ lptr->start = ptr;
+ curqlevel->numvar = 1;
+ state = LQPRS_WAITDELIM;
+ if (charlen == 1)
+ {
+ if (t_iseq(ptr, '\\'))
{
- GETVAR(curqlevel) = lptr = (nodeitem *) palloc0(sizeof(nodeitem) * numOR);
- lptr->start = ptr;
- curqlevel->numvar = 1;
state = LQPRS_WAITESCAPED;
+ break;
}
- else if (strchr(".|@%{}", *ptr))
+ if (t_iseq(ptr, '!'))
{
- UNCHAR;
+ lptr->start += 1 /*FIXME explain why */;
+ curqlevel->flag |= LQL_NOT;
+ hasnot = true;
}
- else
+ else if (t_iseq(ptr, '"'))
{
- GETVAR(curqlevel) = lptr = (nodeitem *) palloc0(sizeof(nodeitem) * numOR);
- lptr->start = ptr;
- state = LQPRS_WAITDELIM;
- curqlevel->numvar = 1;
- if (t_iseq(ptr, '"'))
- {
- lptr->flag |= LVAR_QUOTEDPART;
- }
+ lptr->flag |= LVAR_QUOTEDPART;
}
}
- else
- {
- GETVAR(curqlevel) = lptr = (nodeitem *) palloc0(sizeof(nodeitem) * numOR);
- lptr->start = ptr;
- state = LQPRS_WAITDELIM;
- curqlevel->numvar = 1;
- }
- }
- else if (state == LQPRS_WAITVAR)
- {
+ break;
+ case LQPRS_WAITVAR:
if (t_isspace(ptr))
{
ptr += charlen;
@@ -596,9 +582,9 @@ lquery_in(PG_FUNCTION_ARGS)
state = (t_iseq(ptr, '\\')) ? LQPRS_WAITESCAPED : LQPRS_WAITDELIM;
if (t_iseq(ptr, '"'))
lptr->flag |= LVAR_QUOTEDPART;
- }
- else if (state == LQPRS_WAITDELIM || state == LQPRS_WAITDELIMSTRICT)
- {
+ break;
+ case LQPRS_WAITDELIM:
+ case LQPRS_WAITDELIMSTRICT:
if (charlen == 1 && t_iseq(ptr, '"'))
{
/* We are here if variant begins with ! */
@@ -725,9 +711,8 @@ lquery_in(PG_FUNCTION_ARGS)
tail_space_symbols = 0;
tail_space_bytes = 0;
}
- }
- else if (state == LQPRS_WAITOPEN)
- {
+ break;
+ case LQPRS_WAITOPEN:
if (charlen == 1 && t_iseq(ptr, '{'))
state = LQPRS_WAITFNUM;
else if (charlen == 1 && t_iseq(ptr, '.'))
@@ -739,9 +724,8 @@ lquery_in(PG_FUNCTION_ARGS)
}
else
UNCHAR;
- }
- else if (state == LQPRS_WAITFNUM)
- {
+ break;
+ case LQPRS_WAITFNUM:
if (charlen == 1 && t_iseq(ptr, ','))
state = LQPRS_WAITSNUM;
else if (t_isdigit(ptr))
@@ -751,9 +735,8 @@ lquery_in(PG_FUNCTION_ARGS)
}
else
UNCHAR;
- }
- else if (state == LQPRS_WAITSNUM)
- {
+ break;
+ case LQPRS_WAITSNUM:
if (t_isdigit(ptr))
{
curqlevel->high = atoi(ptr);
@@ -766,16 +749,14 @@ lquery_in(PG_FUNCTION_ARGS)
}
else
UNCHAR;
- }
- else if (state == LQPRS_WAITCLOSE)
- {
+ break;
+ case LQPRS_WAITCLOSE:
if (charlen == 1 && t_iseq(ptr, '}'))
state = LQPRS_WAITEND;
else if (!t_isdigit(ptr))
UNCHAR;
- }
- else if (state == LQPRS_WAITND)
- {
+ break;
+ case LQPRS_WAITND:
if (charlen == 1 && t_iseq(ptr, '}'))
{
curqlevel->high = curqlevel->low;
@@ -785,9 +766,8 @@ lquery_in(PG_FUNCTION_ARGS)
state = LQPRS_WAITSNUM;
else if (!t_isdigit(ptr))
UNCHAR;
- }
- else if (state == LQPRS_WAITEND)
- {
+ break;
+ case LQPRS_WAITEND:
if (charlen == 1 && (t_iseq(ptr, '.') || t_iseq(ptr, '|')))
{
state = LQPRS_WAITLEVEL;
@@ -795,16 +775,15 @@ lquery_in(PG_FUNCTION_ARGS)
}
else
UNCHAR;
- }
- else if (state == LQPRS_WAITESCAPED)
- {
+ break;
+ case LQPRS_WAITESCAPED:
state = LQPRS_WAITDELIM;
escaped_count++;
- }
- else
+ break;
+ default:
/* internal error */
elog(ERROR, "internal error in parser");
-
+ }
ptr += charlen;
if (state == LQPRS_WAITDELIM || state == LQPRS_WAITDELIMSTRICT)
lptr->wlen++;
