sysprog21/shecc

Parse syntax for include macro in parser.c

cratuki opened this issue · 5 comments

cratuki commented

Concern about code from line 3288 in parser.c,

    if (lex_peek(T_include, token)) {
        if (!strcmp(token_str, "<stdio.h>")) {
            /* ignore, we include libc by default */
        }
        lex_expect(T_include);

If the comment is valid, the lex_expect line should be inside the if block. As it is, stdio.h include lines will be processed by a lex_expect call.

If the comment is valid, here is a change that would put the lex_expect is put in an else block, and removing negation on the strcmp clause,

    if (lex_peek(T_include, token)) {
        if (strcmp(token_str, "<stdio.h>")) {
            /* ignore, we include libc by default */
        } else {
            lex_expect(T_include);
        }
vacantron commented

In shecc lexer, the whole line with the compler directive include is regarded as a single "include statement" instead of the combination of "include keyword + identifier (file path)".

shecc/src/lexer.c

Lines 255 to 261 in ba161c5

if (!strcmp(token_str, "#include")) {
do {
token_str[i++] = next_char;
} while (read_char(0) != '\n');
skip_whitespace();
return T_include;
}

For example, the line #include "example.c"\n after lexer reading will be #include"example.c" and report it is a "include" token, and the string will be stored in the token_str. So, the correct change in the parser should be:

    if (lex_peek(T_include, token)) {
        if (!strcmp(token_str, "#include<stdio.h>")) {  /* or we could reuse the variable "token" to get rid of the global variable "token_str" */
            /* ignore, we include libc by default */
        }
        lex_expect(T_include);
}

If the comment is valid, the lex_expect line should be inside the if block. As it is, stdio.h include lines will be processed by a lex_expect call.

Actually, this part of code is only to skip the include statement. The actual source file loading is in the load_source_file():

shecc/src/parser.c

Lines 2620 to 2634 in ba161c5

if (!strncmp(buffer, "#include ", 9) && (buffer[9] == '"')) {
char path[MAX_LINE_LEN];
int c = strlen(file) - 1;
while (c > 0 && file[c] != '/')
c--;
if (c) {
/* prepend directory name */
strncpy(path, file, c + 1);
c++;
}
path[c] = 0;
buffer[strlen(buffer) - 2] = 0;
strcpy(path + c, buffer + 10);
load_source_file(path);
} else {

jserv commented

@cratuki, Unless there are additional concerns, I am inclined to consider this issue resolved.

cratuki commented

  1. I understand why my suggested fix is not valid. @vacantron has explained that the purpose of the outer if (lex_peek) is to skip the include line.

  2. In this case, I suggest that the if statement containing strcmp should be removed, and its comment also. Function strcmp is a pure function, and there is no content within the if clause. Hence, it would improve the code to remove it. Shall I raise a PR?

cratuki commented

I do not have permissions to raise a PR.

The quote below shows what I suggest,

m75-office-001$ git show
commit e33a621376da1280ae0d986975434af56587986e (HEAD -> 20240115.cturner.simplify.parser, fork/20240115.cturner.simplify.parser)
Author: Craig Turner <cratuki@users.noreply.github.com>
Date:   Mon Jan 15 10:49:36 2024 +0100

    Removes redundant block from parser.c

diff --git a/src/parser.c b/src/parser.c
index 569ad89..d544bd2 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -2386,9 +2386,6 @@ void read_global_statement()
     block_t *block = &BLOCKS[0]; /* global block */
 
     if (lex_peek(T_include, token)) {
-        if (!strcmp(token_str, "<stdio.h>")) {
-            /* ignore, we include libc by default */
-        }
         lex_expect(T_include);
     } else if (lex_accept(T_define)) {
         char alias[MAX_VAR_LEN];
m75-office-001$ gh pr create -f -r vacantron,ChAoSUnItY,jserv
? Where should we push the '20240115.cturner.simplify.parser' branch? Create a fork of sysprog21/shecc

Creating pull request for cratuki:20240115.cturner.simplify.parser into master in sysprog21/shecc

remote: 
remote: 
To github.com:cratuki/shecc.git
 * [new branch]      HEAD -> 20240115.cturner.simplify.parser
Branch '20240115.cturner.simplify.parser' set up to track remote branch '20240115.cturner.simplify.parser' from 'fork'.
https://github.com/sysprog21/shecc/pull/105
pull request update failed: GraphQL: cratuki does not have the correct permissions to execute `RequestReviews` (requestReviews)
m75-office-001$
jserv commented
  1. In this case, I suggest that the if statement containing strcmp should be removed, and its comment also. Function strcmp is a pure function, and there is no content within the if clause. Hence, it would improve the code to remove it. Shall I raise a PR?

As discussed previously on Hacker News, I am a university faculty member, and the primary motivation behind starting this project was to establish a hands-on learning platform for my students. Your pull request presents an excellent opportunity for them to engage in code review and collaborative practices, which I greatly value and appreciate.

  • ❤️1