Skip to content

Conversation

ulmer-a
Copy link
Collaborator

@ulmer-a ulmer-a commented Jul 22, 2021

This will resolve #196

  • expression_parser, statement_parser: PResult mostly removed, it is however left in some function to avoid code duplication.
  • parser: ...

@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #202 (c9b45c5) into master (862f5e7) will increase coverage by 0.01%.
The diff coverage is 98.75%.

❗ Current head c9b45c5 differs from pull request most recent head ac58767. Consider uploading reports for the commit ac58767 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   95.86%   95.88%   +0.01%     
==========================================
  Files          54       54              
  Lines       21044    21158     +114     
==========================================
+ Hits        20174    20287     +113     
- Misses        870      871       +1     
Impacted Files Coverage Δ
src/codegen/tests.rs 92.85% <ø> (ø)
src/lexer.rs 36.56% <ø> (-1.22%) ⬇️
...tests/parse_errors/parse_error_containers_tests.rs 89.53% <90.90%> (ø)
src/parser.rs 97.34% <94.87%> (-1.27%) ⬇️
src/index/tests/index_tests.rs 99.57% <100.00%> (ø)
src/lib.rs 42.10% <100.00%> (+1.87%) ⬆️
src/parser/control_parser.rs 95.88% <100.00%> (+3.93%) ⬆️
src/parser/expressions_parser.rs 97.12% <100.00%> (+0.07%) ⬆️
src/parser/tests/container_parser_tests.rs 100.00% <100.00%> (ø)
src/parser/tests/control_parser_tests.rs 99.67% <100.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 862f5e7...ac58767. Read the comment docs.

@ulmer-a ulmer-a marked this pull request as ready for review July 26, 2021 09:12
let condition = parse_expression(lexer);
lexer.expect(KeywordThen)?;
let condition = parse_primary_expression(lexer);
expect_token!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this:
You can use parse in region or a similar method (there's one for statements) with a closing condition KeywordThen

Copy link
Collaborator

Choose a reason for hiding this comment

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

the parse_in_region is not good for this situation because it will try to recover until the next safe end-keywords eating everything in between:

IF condition
    call1();
    call2();
 END_IF

the missing THEN would try to recover until the END_IF and ignore call1() and call2()

take a look how the parse_while_statement(...) treats the situation:

WHILE condition DO
     ...
END_WHILE

it enforces an expression after the while (and reports manually if it cannot find any) and consume_or_report the DO-keyword therefore surviving a missing DO-keyword.
only then it enters a end-while region (although maybe all of this could already be wrapped into the end-while-region)

let start = lexer.range().start;
lexer.advance(); // FOR

let counter_expression = parse_reference(lexer)?;
lexer.expect(KeywordAssignment)?; // :=
let counter_expression = parse_reference(lexer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you formulate this as follows I think you can avoid the expect:
After the FOR keyword, you start a region that ends with DO
Inside this region you will get tokens for the Assignment, TO and BY. I think you can treat these as consume or report instances.
You would need to cover these by tests though (The for is an assignment, followed by a TO Reference, followed optionally by a BY reference and then closed by a DO)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its a similar situation like in the first comment - but I'm not sure if I got everything right from your suggestion.
It would also be doable like the while again

let ref = match parse_reference(...) {
    Err(..) => report meaningful error in the current context, return empty-statement
    Ok(it) => it
};
allow(TO);
... 

maybe you could even create a fancy method or macro for it

let selector = Box::new(parse_expression(lexer)?);
let selector = Box::new(parse_primary_expression(lexer));

expect_token!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can treat this as a region that ends with the OF

@ulmer-a
Copy link
Collaborator Author

ulmer-a commented Jul 27, 2021

I will close this as a dead end and open a new pull request where I will build upon commit c9b45c5, since I already committed changes on this branch that turned out not suitable.

@ulmer-a ulmer-a closed this Jul 27, 2021
@ulmer-a ulmer-a mentioned this pull request Jul 27, 2021
@ulmer-a ulmer-a deleted the p_result branch August 6, 2021 12:26
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.

Remove PResult where possible in parser
3 participants