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

["Carlos Maniero" <carlos@maniero.me>]

> +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.

> +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.

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

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.

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) {
             ++line.size;
         }

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

> +    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.

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