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.

Sign up for GitHub

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

Jump to bottom

Always emit non-logical newlines for 'empty' lines #27

Merged
merged 1 commit into from
May 15, 2023
Merged

Always emit non-logical newlines for 'empty' lines #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions parser/src/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,10 @@ where
}
Some('\n' | '\r') => {
// Empty line!
let tok_start = self.get_pos();
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to your changes. I think this emits two newlines when using \r\n instead of one.

Copy link
Member

Choose a reason for hiding this comment

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

I think this emits two newlines when using \r\n instead of one

yeah, that should be the case, could probably special case this by looking at the next char and just advancing but can't remember if that caused issues when I was tweaking this a while ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might actually work correctly? I think next_char already does the advancement:

// Helper function to go to the next character coming up.
fn next_char(&mut self) -> Option<char> {
    let mut c = self.window[0];
    self.window.slide();
    match c {
        Some('\r') => {
            if self.window[0] == Some('\n') {
                self.location += TextSize::from(1);
                self.window.slide();
            }

            self.location += TextSize::from(1);
            c = Some('\n');
        }
        #[allow(unused_variables)]
        Some(c) => {
            self.location += c.text_len();
        }
        _ => {}
    }
    c
}

Copy link
Member

@DimitrisJim DimitrisJim May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ah, I had forgotten about that 😄 Test would also probably fail if this was the this case.

self.next_char();
let tok_end = self.get_pos();
self.emit((Tok::NonLogicalNewline, TextRange::new(tok_start, tok_end)));
spaces = 0;
tabs = 0;
}
Expand Down Expand Up @@ -1121,7 +1124,7 @@ where
return Err(LexicalError {
error: LexicalErrorType::LineContinuationError,
location: self.get_pos(),
})
});
}
}

Expand Down Expand Up @@ -1501,6 +1504,7 @@ mod tests {
Tok::Return,
Tok::Int { value: BigInt::from(99) },
Tok::Newline,
Tok::NonLogicalNewline,
Tok::Dedent,
]
);
Expand Down Expand Up @@ -1540,10 +1544,12 @@ mod tests {
},
Tok::Colon,
Tok::Newline,
Tok::NonLogicalNewline,
Tok::Indent,
Tok::Return,
Tok::Int { value: BigInt::from(99) },
Tok::Newline,
Tok::NonLogicalNewline,
Tok::Dedent,
Tok::Dedent,
]
Expand Down Expand Up @@ -1578,10 +1584,12 @@ mod tests {
},
Tok::Colon,
Tok::Newline,
Tok::NonLogicalNewline,
Tok::Indent,
Tok::Return,
Tok::Int { value: BigInt::from(99) },
Tok::Newline,
Tok::NonLogicalNewline,
Tok::Dedent,
Tok::Dedent,
]
Expand Down Expand Up @@ -1699,15 +1707,15 @@ mod tests {

#[test]
fn test_logical_newline_line_comment() {
let source = "#Hello\n#World";
let source = "#Hello\n#World\n";
let tokens = lex_source(source);
assert_eq!(
tokens,
vec![
Tok::Comment("#Hello".to_owned()),
// tokenize.py does put an NL here...
Tok::NonLogicalNewline,
Tok::Comment("#World".to_owned()),
// ... and here, but doesn't seem very useful.
Tok::NonLogicalNewline,
]
);
}
Expand Down