-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/add enum support #19
base: main
Are you sure you want to change the base?
Feature/add enum support #19
Conversation
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.
Thank you for the contribution! It's looking great and we're really happy with the direction you've taken this is. There are some stylistic items the review found and a couple of queries on logic/symbol handling, but once sorted, we're happy to merge this.
@@ -1854,6 +1924,8 @@ export class Parser | |||
const token = this.lexer.token | |||
if (config.allowExtStmt && token.typeIsOneOf(TokenType.visibility)) | |||
return this.parseVisibility() | |||
if (config.isEnum) |
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.
This should probably check && token.typeIsOneOf(TokenType.enumDef)
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.
So the only token that's considered the enum type is the initial enum keyword, we also can't really force every token inside the enum to be of the enum type as when we parse the value assignments to the enum members they need to have their own typing, seperate to the enum.
Since the isEnum config key is set only once we've read the enum keyword and continue to parse the braces we thought this would be fine as no other routes could lead to this being reached and parsing anything else incorrectly, let us know if you have any thoughts on this one
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.
It comes down to: Do we want to only allow enums to contain enumeration values, or do we want to allow them to contain conditional logic, visibility statements, functions, operator overloads, etc?
One of the biggest problems C++ has always had with its enumerations (after fixing the scope problem) is there's no way to directly attach functionality to the type such as helpers to string-ify the enumeration values, and operators to help build complex values from enumerated values. If you agree that can make sense here, and given this current setup does allow visibility statements already, then we think checking for TokenType.enumDef is correct, otherwise we agree with the restriction
aeec4d4
to
cda000b
Compare
7e8b8b3
to
8384bce
Compare
8384bce
to
cd6bc73
Compare
Hmm learning more about how github handles force pushes and history rewrites, sorry that log is a bit of a mess >< Anyway as part of this set of changes I've also updated instances of enumValues -> enumMember so that the wording is consistent with the bnf, let me know if there's anything further |
type = 0x8000 | ||
bool = 0x000200, | ||
enum = 0x000400, | ||
class = 0x00800, |
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.
class
is struct
above as while the keyword is class
, the language defines them as structures-with-members so internally we've been using struct
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.
There is an item that cropped up in this pass, but once that and the outstanding items from the previous are resolved, we think this is ready for merge - excellent work.
@@ -1854,6 +1924,8 @@ export class Parser | |||
const token = this.lexer.token | |||
if (config.allowExtStmt && token.typeIsOneOf(TokenType.visibility)) | |||
return this.parseVisibility() | |||
if (config.isEnum) |
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.
It comes down to: Do we want to only allow enums to contain enumeration values, or do we want to allow them to contain conditional logic, visibility statements, functions, operator overloads, etc?
One of the biggest problems C++ has always had with its enumerations (after fixing the scope problem) is there's no way to directly attach functionality to the type such as helpers to string-ify the enumeration values, and operators to help build complex values from enumerated values. If you agree that can make sense here, and given this current setup does allow visibility statements already, then we think checking for TokenType.enumDef is correct, otherwise we agree with the restriction
When you get to addressing the most resent review, please rebase against main so we can cleanly merge this with signatures kept intact |
Detailed description
This adds in parsing and syntax highlighting for enums
I've been testing against the following definition:
Your checklist for this pull request
Closing issues