Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hCjVa-0000YZ-N3 for pgsql-hackers@arkaria.postgresql.org; Sat, 06 Apr 2019 11:29:15 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hCjVY-0006f1-6K for pgsql-hackers@arkaria.postgresql.org; Sat, 06 Apr 2019 11:29:12 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hCjVX-0006eu-UP for pgsql-hackers@lists.postgresql.org; Sat, 06 Apr 2019 11:29:11 +0000 Received: from ssl.nataraj.su ([195.54.209.170]) by magus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hCjVR-0000Zg-SU for pgsql-hackers@lists.postgresql.org; Sat, 06 Apr 2019 11:29:11 +0000 Received: from x200m.localnet (unknown [176.59.21.112]) by ssl.nataraj.su (Postfix) with ESMTPSA id D0481511E23C; Sat, 6 Apr 2019 14:29:03 +0300 (MSK) From: Nikolay Shaplov To: pgsql-hackers@lists.postgresql.org Cc: Dmitry Belyavsky Subject: Re: Ltree syntax improvement Date: Sat, 06 Apr 2019 14:28:59 +0300 Message-ID: <2996702.6AG0pjhCY8@x200m> User-Agent: KMail/5.2.3 (Linux/4.9.0-8-amd64; KDE/5.28.0; x86_64; ; ) In-Reply-To: References: <1659194.9VRYtGT3Pi@x200m> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart21296630.ADSjtgLaXs" Content-Transfer-Encoding: 7Bit List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --nextPart21296630.ADSjtgLaXs Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" =D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=B2=D0=BE=D1=81= =D0=BA=D1=80=D0=B5=D1=81=D0=B5=D0=BD=D1=8C=D0=B5, 24 =D1=84=D0=B5=D0=B2=D1= =80=D0=B0=D0=BB=D1=8F 2019 =D0=B3. 14:31:55 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0= =B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Dmitry=20 Belyavsky =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB: Hi! Am back here again. I've been thinking about this patch a while... Come to some conclusions and= =20 wrote some examples... =46irst I came to idea that the best way to simplify the code is change the= =20 state machine from if to switch/case. Because in your code a lot of repetit= ion=20 is done just because you can't say "Thats all, let's go to next symbol" in = any=20 place in the code. Now it should follow all the branches of if-else tree th= at=20 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 process= ing=20 in lquery_in form if to switch/case, and changed the code for LQPRS_WAITLEV= EL=20 state processing, removing duplicate code, using "break" wherever it is=20 possible (The indention in this example is unproper to make diff more clear) so from that much of code =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D if (state =3D=3D LQPRS_WAITLEVEL) { if (t_isspace(ptr)) { ptr +=3D charlen; pos++; continue; } escaped_count =3D 0; real_levels++; if (charlen =3D=3D 1) { if (t_iseq(ptr, '!')) { GETVAR(curqlevel) =3D lptr =3D (nodeitem *)=20 palloc0(sizeof(nodeitem) * numOR); lptr->start =3D ptr + 1; state =3D LQPRS_WAITDELIM; curqlevel->numvar =3D 1; curqlevel->flag |=3D LQL_NOT; hasnot =3D true; } else if (t_iseq(ptr, '*')) state =3D LQPRS_WAITOPEN; else if (t_iseq(ptr, '\\')) { GETVAR(curqlevel) =3D lptr =3D (nodeitem *)=20 palloc0(sizeof(nodeitem) * numOR); lptr->start =3D ptr; curqlevel->numvar =3D 1; state =3D LQPRS_WAITESCAPED; } else if (strchr(".|@%{}", *ptr)) { UNCHAR; } else { GETVAR(curqlevel) =3D lptr =3D (nodeitem *)=20 palloc0(sizeof(nodeitem) * numOR); lptr->start =3D ptr; state =3D LQPRS_WAITDELIM; curqlevel->numvar =3D 1; if (t_iseq(ptr, '"')) { lptr->flag |=3D LVAR_QUOTEDPART; } } } else { GETVAR(curqlevel) =3D lptr =3D (nodeitem *) palloc0(sizeof(nodeitem) *= =20 numOR); lptr->start =3D ptr; state =3D LQPRS_WAITDELIM; curqlevel->numvar =3D 1; } } =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D I came to this =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D case LQPRS_WAITLEVEL: if (t_isspace(ptr)) break; /* Just go to next symbol */ escaped_count =3D 0; real_levels++; if (charlen =3D=3D 1) { if (strchr(".|@%{}", *ptr)) UNCHAR; if (t_iseq(ptr, '*')) { state =3D LQPRS_WAITOPEN; break; } } GETVAR(curqlevel) =3D lptr =3D (nodeitem *) palloc0(sizeof(nodeitem) *=20 numOR); lptr->start =3D ptr; curqlevel->numvar =3D 1; state =3D LQPRS_WAITDELIM; if (charlen =3D=3D 1) { if (t_iseq(ptr, '\\')) { state =3D LQPRS_WAITESCAPED; break; } if (t_iseq(ptr, '!')) { lptr->start +=3D 1 /*FIXME explain why */; curqlevel->flag |=3D LQL_NOT; hasnot =3D true; } else if (t_iseq(ptr, '"')) { lptr->flag |=3D LVAR_QUOTEDPART; } } break; =3D=3D=3D=3D=3D=3D=3D And this code is much more readable.... You can just read it aloud: =2DSpaces we just skip =2D on .|@%{} throws and exception =2D for asterisk change state and nothing else then for others allocate a buffer =2D for slash we just set a flag =2D for exclamation mark we set a flag and skip it =2D for double quote set a flag. All the logic are clear. And in your version of the code it is difficult to= get=20 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 swit= ch/ cases, and nothing else. This will reindent the code, so in order to keep=20 further changes visible, we should change nothing else. 2. Change your code to switch/cases, use break to deduplicate code wherever= is=20 possible, and commit it. I can join you while working with this (but then I am not sure I would be a= ble=20 to remain a reviewer...) I've also attached an example I've quoted above, formatted as a diff, so it= =20 would be easy to check how does it work. It should be applied after your=20 patch. (I gave it a strange name ltree.not-a-patch hoping that commitfest software= =20 will not treat it a new version of a patch) --nextPart21296630.ADSjtgLaXs Content-Disposition: attachment; filename="ltree.not-a-patch" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="UTF-8"; name="ltree.not-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++; --nextPart21296630.ADSjtgLaXs--