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

Feature/add enum support #19

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

SludgeGirl
Copy link
Contributor

Detailed description

This adds in parsing and syntax highlighting for enums

I've been testing against the following definition:

[[FlagsEnum]]
enum ConsoleOptions
{
	debug = 100,
	testing = 200,
	test1
}

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds and lints cleanly
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do
  • I've made sure that my commits are signed/verified

Closing issues

@dragonmux dragonmux added the Enhancement General project improvement label May 9, 2023
Copy link
Member

@dragonmux dragonmux left a 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.

src/server/ast/statements.ts Outdated Show resolved Hide resolved
src/server/ast/statements.ts Outdated Show resolved Hide resolved
src/server/ast/statements.ts Outdated Show resolved Hide resolved
src/server/ast/symbolTable.ts Outdated Show resolved Hide resolved
src/server/parser/parser.ts Outdated Show resolved Hide resolved
src/server/parser/parser.ts Outdated Show resolved Hide resolved
src/server/parser/parser.ts Outdated Show resolved Hide resolved
src/server/parser/parser.ts Outdated Show resolved Hide resolved
@@ -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)
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Member

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

@SludgeGirl SludgeGirl force-pushed the feature/add-enum-support branch 2 times, most recently from aeec4d4 to cda000b Compare June 1, 2023 09:34
@SludgeGirl SludgeGirl force-pushed the feature/add-enum-support branch 3 times, most recently from 7e8b8b3 to 8384bce Compare June 1, 2023 09:45
@SludgeGirl
Copy link
Contributor Author

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

@SludgeGirl SludgeGirl requested a review from dragonmux June 1, 2023 10:02
type = 0x8000
bool = 0x000200,
enum = 0x000400,
class = 0x00800,
Copy link
Member

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

src/server/ast/symbolTable.ts Outdated Show resolved Hide resolved
Copy link
Member

@dragonmux dragonmux left a 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)
Copy link
Member

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

@dragonmux
Copy link
Member

When you get to addressing the most resent review, please rebase against main so we can cleanly merge this with signatures kept intact

@dragonmux dragonmux added Component: Parser Something related to the language parser Component: AST Something related to the parsed abstract syntax tree labels Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: AST Something related to the parsed abstract syntax tree Component: Parser Something related to the language parser Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants