Re: Ltree syntax improvement

Dear Nikolay,

Many thanks for your efforts!

On Sat, Apr 6, 2019 at 2:29 PM Nikolay Shaplov <dhyan@nataraj.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