Skip to content

Fix the tokenization of < edge cases#2280

Open
ayman-sigma wants to merge 1 commit intoapache:mainfrom
ayman-sigma:ayman/fixLtTokenizer
Open

Fix the tokenization of < edge cases#2280
ayman-sigma wants to merge 1 commit intoapache:mainfrom
ayman-sigma:ayman/fixLtTokenizer

Conversation

@ayman-sigma
Copy link
Contributor

This change fixes tokenizer behavior for < prefixed sequences so subtraction and comparison are parsed correctly instead of being treated as an operator start. The issue mainly happens when there is no space between < and other tokens like -, or =.

This PR address:

  1. The use of + or - after < (Lt) to identify the sign of the following number/identifier
  2. Similarly, the use of + or - after <= (LtEq)
  3. Keeps the Two way arrow token <-> the same for dialects that supports geometric types.

Comment on lines +1630 to +1631
Some('+') => Ok(Some(Token::LtEq)),
Some('-') => Ok(Some(Token::LtEq)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some('+') => Ok(Some(Token::LtEq)),
Some('-') => Ok(Some(Token::LtEq)),
Some('+') | Some('-') => Ok(Some(Token::LtEq)),

Could we include a comment flagging why these two operators are specially overridden here?

}
_ => self.start_binop_opt(chars, "<-", None),
if chars.peekable.clone().nth(1) == Some('>') {
chars.next(); // consume
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
chars.next(); // consume
chars.next(); // consume `-`

otherwise it looks on first read like we're consuming > which seemed incorrect

}
}
Some('<') => self.consume_for_binop(chars, "<<", Token::ShiftLeft),
Some('+') => Ok(Some(Token::Lt)),
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly it would be nice to flag the special cases in a comment

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.

2 participants