Re: [PATCH olang v1 4/4] parser: create simplified parser for tiny AST

[Johnny Richard <johnny@johnnyrichard.com>]

On Fri, Mar 01, 2024 at 12:34:59AM -0300, Carlos Maniero wrote:
> > +ast_node_t *
> > +ast_make_node_fn_def(arena_t *arena, string_view_t identifier, type_t return_type, ast_node_t *block);
> > +
> > +ast_node_t *
> > +ast_make_node_literal_u32(arena_t *arena, uint32_t value);
> > +
> > +ast_node_t *
> > +ast_make_node_return_stmt(arena_t *arena);
> > +
> > +ast_node_t *
> > +ast_make_node_block(arena_t *arena);
> 
> s/ast_make/ast_new just to keep consistency with other functions that
> allocate memory.

Sure!

> > +void
> > +lexer_print_token_highlight(lexer_t *lexer, token_t *token, FILE *stream)
> > +{
> > +    size_t offset = token->location.bol;
> > +    char *str = lexer->source.chars + offset;
> > +
> > +    size_t i = 0;
> > +    while ((i + offset) < lexer->source.size && str[i] != '\n' && str[i] != 0) {
> > +        ++i;
> > +    }
> > +    string_view_t line = { .chars = str, .size = i };
> > +    fprintf(stream, "" SV_FMT "\n", SV_ARG(line));
> > +    fprintf(stream, "%*s\n", (int)(token->location.offset - token->location.bol + 1), "^");
> > +}
> 
> 1. It bothers me a little that the lexer is performing IO operations.
>    IMO, the parser should be responsible for error handling. But I can live
>    with this if you think this is the right place for this function.

After analise the code and think about the future typechecker
implementation I kind agree with you, I will change in the next patch.

> 2. An alternative is making the lexer returning the line *string_view*
>    and make the parser do the rest.

Great, I will do that.

> 3. nitpick: Isn't *(i + offset) < lexer->source.size* and *str[i] != 0*
>    redundant? The last char will (or at least should) always be a NULL
>    terminator.

I will keep the check, I don't see a problem on keeping it.  This will
make the code more resilient in case of bugs.

> 4. I spent some time trying to understand what this function was
>    supposed to do. I believe that having a function that just returns
>    the token's line could help (2). But an alternative here is using the
>    *string_view* struct instead of having *str* and *i*. Something like
>    this:
> 
>      void
>      lexer_print_token_highlight(lexer_t *lexer, token_t *token, FILE *stream)
>      {
>          size_t offset = token->location.bol;
>          string_view_t line = { .chars = lexer->source.chars + offset, .size = 0 };
>      
>          while (line.chars[i] != '\n' && line.chars[i] != 0) {

Ops, you forgot to remove the **i** here. :x

>              ++line.size;
>          }
> 
>          fprintf(stream, "" SV_FMT "\n", SV_ARG(line));
>          fprintf(stream, "%*s\n", (int)(token->location.offset - token->location.bol + 1), "^");
>      }

Yeah, I will do this change as well, thanks.

> > +    if (!skip_expected_token(parser, TOKEN_COLON))
> > +        return NULL;
> > +
> > +    skip_line_feeds(parser->lexer);
> > +
> > +    type_t fn_return_type;
> > +    if (!parser_parse_type(parser, &fn_return_type)) {
> > +        return NULL;
> > +    }
>
> nitpick: I’ve spotted some inconsistency with the use of brackets in
> if statements. Just throwing it out there, but I believe we should be
> using brackets in all if statements. However, I’m totally fine with
> removing them in guard clauses, as long as we maintain a consistent
> style.

Let's be consistent and keep **{** and **}** on every flow control
statements. Changed.

> This is great work man! I'm excited of how close we are now from start
> the back-end.

Thanks for reviewing it. I will make almost every change you requested
on a new patch revision.