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

Add macro declaration syntax. #7075

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

gilbens-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 14 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 9 of 15 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware)


crates/cairo-lang-parser/src/parser.rs line 1487 at r2 (raw file):

            SyntaxKind::TerminalDiv => self.take::<TerminalDiv>().into(),
            SyntaxKind::TerminalDivEq => self.take::<TerminalDivEq>().into(),
            SyntaxKind::TerminalDollar => self.take::<TerminalDollar>().into(),

should dollar be not fully part of the identifier?

Code quote:

            SyntaxKind::TerminalDollar => self.take::<TerminalDollar>().into(),

crates/cairo-lang-parser/src/parser.rs line 1861 at r2 (raw file):

    /// Parses an expr block, while allowing placeholder expressions. Restores the previous
    /// placeholder expression setting after parsing.
    fn parse_block_with_placeholders(&mut self) -> ExprBlockGreen {

doesn't this have exactly a single caller?

Code quote:

 parse_block_with_placeholders(

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 15 files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-parser/src/parser.rs line 1487 at r2 (raw file):

Previously, orizi wrote…

should dollar be not fully part of the identifier?

You mean removing dollar as a token, and allowing it as an identifier character (only in the first position) in the lexer level?
That was my first intention, but then how is $false is lexed? (or any other keyword). I thought of two options, either as an identifier, and we'll need to check for keywords in another place, or as $ and false (similar to what I did, but with $ as an identifier I guess), but I thought it will only add more edge cases, so I opted to always split them.


crates/cairo-lang-parser/src/parser.rs line 1861 at r2 (raw file):

Previously, orizi wrote…

doesn't this have exactly a single caller?

There is, however, it felt safer to wrap it in a function (to prevent turning the flag on and forgetting to turn it off).

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 15 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)


crates/cairo-lang-parser/src/parser.rs line 1487 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

You mean removing dollar as a token, and allowing it as an identifier character (only in the first position) in the lexer level?
That was my first intention, but then how is $false is lexed? (or any other keyword). I thought of two options, either as an identifier, and we'll need to check for keywords in another place, or as $ and false (similar to what I did, but with $ as an identifier I guess), but I thought it will only add more edge cases, so I opted to always split them.

i mostly mean that there shouldn't be a space allowed between the $ and the identifier.

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 15 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-parser/src/parser.rs line 1487 at r2 (raw file):

Previously, orizi wrote…

i mostly mean that there shouldn't be a space allowed between the $ and the identifier.

Asserting that the trailing trivia is empty? It can be done, but it deviates from any other thing that we do with whitespaces.
In rust it works btw, also a trailing comment after the $ works.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 15 files reviewed, all discussions resolved


crates/cairo-lang-parser/src/parser.rs line 1487 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Asserting that the trailing trivia is empty? It can be done, but it deviates from any other thing that we do with whitespaces.
In rust it works btw, also a trailing comment after the $ works.

ok - so just change the formatter at some point to make this non-confusing.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 14 files at r1, 1 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Jan 22, 2025
Merged via the queue into high-level-inline-macros with commit cb8bb62 Jan 22, 2025
45 checks 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.

3 participants