Re: Ltree syntax improvement

Dear Nikolay,

Many thanks for your efforts!

On Sat, Apr 6, 2019 at 2:29 PM Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:

> В письме от воскресенье, 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)

I've applied your patch.
From my point of view, there is no major difference between case and chain
if here.
Neither case nor ifs allow extracting the common code to separate function
- just because there seem to be no identical pieces of code.

So yes, your patch makes sense, but I do not see any advantages of applying
it.

--
SY, Dmitry Belyavsky