From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@lists.postgresql.org
Cc: Dmitry Belyavsky <beldmit@gmail.com>
Subject: Re: Ltree syntax improvement
Date: Sat, 06 Apr 2019 14:28:59 +0300
Message-ID: <2996702.6AG0pjhCY8@x200m> (raw)
In-Reply-To: <CADqLbzK9ke9haVCBN8wAreMGpdvO3qN3jF1SJ7d=g0VC-TxrSw@mail.gmail.com>
References: <CADqLbzLVkBuPX0812o+z=c3i6honszsZZ6VQOSKR3VPbB56P3w@mail.gmail.com>
<1659194.9VRYtGT3Pi@x200m>
<CADqLbzK9ke9haVCBN8wAreMGpdvO3qN3jF1SJ7d=g0VC-TxrSw@mail.gmail.com>
В письме от воскресенье, 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)
Attachments:ltree.not-a-patch (text/x-patch, 4.8K) downloaddiff --git a/.gitignore b/.gitignoreindex 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.cindex 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++;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: dhyan@nataraj.su, pgsql-hackers@lists.postgresql.org, beldmit@gmail.com
Subject: Re: Ltree syntax improvement
In-Reply-To: <2996702.6AG0pjhCY8@x200m>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox