-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more lex tests #36
Add more lex tests #36
Conversation
return 0; | ||
} | ||
|
||
int test_ttype_one_char() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to get some representation for failure paths, e.g. invalid tokens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean as in testing that it fails gracefully and doesn't segfault or something if we ask ttype_string("/* This is not a token. */")
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, testing doesn't segfault, it just prints the assert that failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean that the testing code itself might be broken, I mean testing whether ttype_string
might fail gracefully or just crash or something. I think this is good to test because our system should be robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I see, yup we definitely should have that added. Same PR or later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later is probably fine.
if (isdigit(c)) { | ||
return TT_LITERAL; | ||
} else { | ||
return TT_IDENTIFIER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know that everything not listed is an identifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. I think we might need to check for one character symbols that definitely aren't tokens or identifiers. I can only think of things like the @
symbol that isn't either, so we might want to have error handling for that later
tassert(strcmp(ttype_name(TT_PLUS), "+") == 0); | ||
tassert(strcmp(ttype_name(TT_SIZEOF), "sizeof") == 0); | ||
tassert(strcmp(ttype_name(TT_WHILE), "while") == 0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing this function is great! I am worried that as a lookup table with no information about the ordering of the enum, it is a bit fragile though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Having this is a good canary in the coal mine for names not working. If the tests break, any code that relies on the function will also break. It's a problem with how naming enums works in C in general so we should be careful with the use of the enum + name function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but merge conflicts :(
Oh noooo |
I will fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the tests pass (which see PR #40 for why I'm not testing this myself), this looks good!
Yup, tests pass on this one. Tests however fail on main.
|
No description provided.