Skip to content
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

Added when keyword #43

Merged
merged 8 commits into from
Jan 30, 2025
Merged

Added when keyword #43

merged 8 commits into from
Jan 30, 2025

Conversation

VonTum
Copy link
Collaborator

@VonTum VonTum commented Dec 20, 2024

  • added when literal to grammar
  • updated auto generated tree-sitter configs
  • added "is_when" bool to if-struct

- added when literal to grammar
- updated auto generated tree-sitter configs
- added "is_when" bool to if-struct
@VonTum
Copy link
Collaborator Author

VonTum commented Dec 20, 2024

First half is here: ceeccc6 , if you want to take a look at it @VonTum . The insane amount of line changes are because I had to regenerate the tree-sitter stuff. Is this normal or did I break something?

I will start a merge request once I get the warnings to work.

Curious, from my first look through parser.c, the massive line count difference is explained by differing tree-sitter versions. Perhaps you're using a slightly newer version of tree-sitter do do your generation? Could you check tree-sitter --version for me?

In general though, you don't need to worry about line changes in parser.c or any of the other generated files. It's usually good practise to have a look at the changes, because tree-sitter generates reasonably readable C code, and I especially just look at the upper header, at STATE_COUNT, LARGE_STATE_COUNT, and others to see if a change I've made drastically messes up the generated parser.

Copy link
Collaborator Author

@VonTum VonTum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start! The parser update looks good, all that is left is to add the necessary warning/error, and make the is_generative depend on the keyword, instead of the context.

src/flattening/flatten.rs Outdated Show resolved Hide resolved
let if_id = self.instructions.alloc(Instruction::IfStatement(IfStatement {
condition,
is_when: literal_is_when,
is_generative: condition_is_generative,// TODO `if` vs `when` https://github.com/pc2/sus-compiler/issues/3
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's TODO's like this one scattered around the codebase. I believe you can get rid of is_generative when you add the warning inline here. (Or even have is_generative get the value of literal_is_when, rather than from its context)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have now made the following changes which should resolve this:

match(keyword_is_if, condition_is_generative){
(true, false) => {
self.errors.warn(position_statement_keyword, "Used 'if' in a non generative context, use 'when' instead");
},
(false, true) => {
self.errors.error(position_statement_keyword, "Used 'when' in a generative context, use 'if' instead");
},
(_, _) => ()
}
let if_id = self.instructions.alloc(Instruction::IfStatement(IfStatement {
condition,
is_generative: keyword_is_if,
then_start: FlatID::PLACEHOLDER,
then_end_else_start: FlatID::PLACEHOLDER,
else_end: FlatID::PLACEHOLDER,
}));

@VonTum VonTum changed the title Added when literal Added when keyword Dec 21, 2024
@TomSkarabis
Copy link
Collaborator

@VonTum I think that this is now ready to merge. Do you want me to rebase or merge from master to update the branch?

@VonTum
Copy link
Collaborator Author

VonTum commented Jan 28, 2025

So it appears that because I technically created the PR, I'm not allowed to review it. You live you learn I guess :P

The changes look good, all that is left is properly merging master into it, and then merging the PR. Good work!

@TomSkarabis TomSkarabis marked this pull request as ready for review January 30, 2025 12:21
Copy link
Collaborator Author

@VonTum VonTum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, everything looks good. Great work!

@VonTum VonTum merged commit 4d31a13 into master Jan 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split overloaded if construct into if and when for compiletime and runtime conditions
2 participants