IndexSeek commented on code in PR #1990:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1990#discussion_r2255722803


##########
src/parser/mod.rs:
##########
@@ -11229,6 +11229,30 @@ impl<'a> Parser<'a> {
         }
     }
 
+    /// Parse an optionally signed integer literal.
+    fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
+        let next_token = self.next_token();
+        let (sign, number_token) = match next_token.token {
+            Token::Minus => {
+                let number_token = self.next_token();
+                (-1, number_token)
+            }
+            Token::Plus => {
+                let number_token = self.next_token();
+                (1, number_token)
+            }
+            _ => (1, next_token),
+        };

Review Comment:
   Thanks for the suggestion! It does read cleaner not starting with negation, 
looking back, opening with `if !self.consume_token(&Token::Minus)` felt a bit 
strange. 
   
   I like how your solution still handles the + sign, but I noticed a few 
issues with the logic flow. I think that with an optional plus token `let _ = 
self.consume_token(&Token::Plus);`, it calls self.advance_token() and then 
self.get_current_token(), which could advance twice and potentially skip the 
actual number token. 
   
   I did take a look at `peek_token_ref` and was wondering if we could use it 
like so?
   
   ```rs
       fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
           let is_negative = self.consume_token(&Token::Minus);
   
           if !is_negative {
               let _ = self.consume_token(&Token::Plus);
           }
   
           let current_token = self.peek_token_ref();
           match &current_token.token {
               Token::Number(s, _) => {
                   let s = s.clone();
                   let span_start = current_token.span.start;
                   self.advance_token();
                   let value = Self::parse::<i64>(s, span_start)?;
                   Ok(if is_negative { -value } else { value })
               }
               _ => self.expected_ref("number", current_token),
           }
       }
   ```
   
   I think that should avoid us needing to consume the token until we know we 
have a number. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to