Re: [PATCH olang v2 1/3] lexer: add tokenize support to binary op tokens

[Johnny Richard <johnny@johnnyrichard.com>]

On Sun, Mar 17, 2024 at 09:30:34PM -0300, Carlos Maniero wrote:
> > +            case '=': {
> > +                size_t start_offset = lexer->offset;
> > +
> > +                if (lexer_peek_next_char(lexer) == '=') {
> > +                    lexer_skip_char(lexer);
> > +                    lexer_skip_char(lexer);
> > +                    lexer_init_str_value_token(lexer, token, TOKEN_CMP_EQ, start_offset);
> > +                    return;
> > +                }
> > +
> > +                lexer_init_char_value_token(lexer, token, TOKEN_EQ);
> > +                lexer_skip_char(lexer);
> > +                return;
> > +            }
> 
> We could apply the *maximal munch* [1] here avoiding looking to the
> future by skipping the char right after founding it:

Thanks, I kind agree on removing the necessity of lookahead here.

> 
> +            case '=': {
> +                size_t start_offset = lexer->offset;
> +                lexer_skip_char(lexer);
> +
> +                if (lexer_current_char(lexer) == '=') {
> +                    lexer_skip_char(lexer);
> +                    lexer_init_str_value_token(lexer, token, TOKEN_CMP_EQ, start_offset);
> +                    return;
> +                }
> +
> +                lexer_init_char_value_token(lexer, token, TOKEN_EQ);

This function expect the offset being point to the char in question.  In
your solution we already moved the offset.

I solution for this problem would replace the /lexer_init_char_value_token/ with 
/lexer_init_str_value_token/.  I have sent a new revision for this patch
with the correction.

> +                lexer_skip_char(lexer);

This solution is moving twice to consume a single char.

> +                return;
> +            }
> 
> I know it is a silly change, but this help us to keep the parsing
> process as deterministic as possible.

I don't think it's a silly change, that's a nice improvement, thanks for
suggesting this change.