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

Move validation to parser. #4295

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Move validation to parser. #4295

merged 1 commit into from
Oct 25, 2023

Conversation

ilyalesokhin-starkware
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware commented Oct 23, 2023

This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor Author

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

        let span = TextSpan { start: self.offset, end: self.offset.add_width(self.current_width) };

        validate_literal_number(self.diagnostics, text, span, self.file_id);

Does this make sense?

Is there a simple way to get the text or syntax_node from TerminalLiteralNumberGreen?

Code quote:

        let text = self.peek().text.clone();
        let span = TextSpan { start: self.offset, end: self.offset.add_width(self.current_width) };

        validate_literal_number(self.diagnostics, text, span, self.file_id);

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 2 of 6 files at r1, all commit messages.
Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-parser/src/lib.rs line 15 at r1 (raw file):

pub mod recovery;
pub mod utils;
#[allow(dead_code)]

is this required?


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

    }

    fn try_terminal_literal_number(&mut self) -> TerminalLiteralNumberGreen {

doc


crates/cairo-lang-parser/src/parser_test_data/partial_trees/literal line 140 at r1 (raw file):

 --> dummy_file.cairo:3:21
    let illegal_bin = 0b2;
                    ^^

fix span

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 1 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)


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

/// 1. Is parsable according to its radix.
/// 2. Has properly formatted suffix.
pub fn validate_literal_number(

Are the other pubs required?

Copy link
Contributor

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)


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

            }
        };
        node

Nit

Suggestion:

        match file_id.kind(db.upcast()) {
            FileKind::Module => {
                Parser::parse_file(db.upcast(), &mut diagnostics, file_id, &s).as_syntax_node()
            }
            FileKind::Expr => {
                Parser::parse_file_expr(db.upcast(), &mut diagnostics, file_id, &s).as_syntax_node()
            }
        }

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 2 of 7 files reviewed, 5 unresolved discussions (waiting on @mkaput and @orizi)


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

Previously, mkaput (Marek Kaput) wrote…

Nit

done


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

Previously, orizi wrote…

Are the other pubs required?

Removed the unnecessary ones.


crates/cairo-lang-parser/src/parser_test_data/partial_trees/literal line 140 at r1 (raw file):

Previously, orizi wrote…

fix span

Done.


crates/cairo-lang-parser/src/lib.rs line 15 at r1 (raw file):

Previously, orizi wrote…

is this required?

reverted

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 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @mkaput)


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

    }

    fn take_terminal_short_string(&mut self) -> TerminalShortStringGreen {

doc


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

    }

    fn take_terminal_string(&mut self) -> TerminalStringGreen {

doc

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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

@ilyalesokhin-starkware ilyalesokhin-starkware marked this pull request as ready for review October 25, 2023 10:30
@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue Oct 25, 2023
Merged via the queue into main with commit 30cd7de Oct 25, 2023
38 checks passed
@orizi orizi deleted the ilya/validation branch November 6, 2023 07:01
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