Minor bug fixes - #6
Conversation
Reviewer's GuideThis PR standardizes token enum usage in the parser, relocates negative-number parsing from the lexer to the parser, and strengthens CI workflows by widening branch triggers, optimizing caching, and clarifying dependency steps. Class diagram for Parser and Lexer changesclassDiagram
class Parser {
+bool match(Token::Kind kind)
+void expect(Token::Kind kind, const std::string& errMsg)
+std::unique_ptr<ASTNode> parse()
+std::unique_ptr<ASTNode> parsePrimary()
}
class Lexer {
+Token nextToken()
-Token scanNumber() // negative number logic removed
}
Parser --> Lexer : uses
class Token {
<<enum>>
Kind
}
Flow diagram for negative number parsing responsibility shiftflowchart TD
Lexer-->|Before: handled negative numbers|NumberToken
Parser-->|Now: handles negative numbers|PrimaryExpr
NumberToken-->|Passed to Parser|PrimaryExpr
PrimaryExpr-->|Creates negative literal AST node|ASTNode
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @LowLevelLore - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/Parser.cpp:540` </location>
<code_context>
}
std::unique_ptr<ASTNode> Parser::parsePrimary() {
+ if (currentToken.kind == Token::Kind::Symbol && currentToken.text == "-") {
+ advance();
</code_context>
<issue_to_address>
Consider moving all unary minus handling from parsePrimary to parseUnary and using a match overload to reduce duplication and centralize logic.
Here’s one way to pull the “–”‐in‐front‐of‐literal logic out of `parsePrimary` and into `parseUnary`, collapse duplication, and keep all existing behavior:
1. Remove your new “if (currentToken == ‘–’) { … }” block from `parsePrimary()`.
2. Add an overload of `match` so you can match on both kind+text:
```cpp
bool Parser::match(Token::Kind kind, const std::string &txt) {
if (currentToken.kind == kind && currentToken.text == txt) {
advance();
return true;
}
return false;
}
```
3. Change `parseUnary()` to handle “–” in one place. If it’s directly in front of a literal you’ll still get a single literal node; if it’s in front of any other expression you get a proper unary‐negate node:
```cpp
std::unique_ptr<ASTNode> Parser::parseUnary() {
// special‐case negative literal:
if (match(Token::Kind::Symbol, "-") &&
(currentToken.kind == Token::Kind::IntegerLiteral ||
currentToken.kind == Token::Kind::FloatLiteral))
{
// capture "-" + literal text, then let primary do the rest
std::string txt = "-" + currentToken.text;
auto kind = currentToken.kind;
advance();
return (kind == Token::Kind::IntegerLiteral)
? ASTNode::makeIntegerLiteralNode(txt, currentScope)
: ASTNode::makeFloatLiteralNode (txt, currentScope);
}
// generic unary operator (e.g. negate a sub‐expr)
if (match(Token::Kind::Symbol, "-")) {
auto operand = parseUnary();
return ASTNode::makeUnaryOperatorNode(UnaryOp::Negate,
std::move(operand),
currentScope);
}
// fall back to primary
return parsePrimary();
}
```
That eliminates the nested `if(“-”)` in `parsePrimary` and centralizes **all** unary‐minus handling in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -538,27 +538,40 @@ namespace zust { | |||
| } | |||
|
|
|||
| std::unique_ptr<ASTNode> Parser::parsePrimary() { | |||
There was a problem hiding this comment.
issue (complexity): Consider moving all unary minus handling from parsePrimary to parseUnary and using a match overload to reduce duplication and centralize logic.
Here’s one way to pull the “–”‐in‐front‐of‐literal logic out of parsePrimary and into parseUnary, collapse duplication, and keep all existing behavior:
- Remove your new “if (currentToken == ‘–’) { … }” block from
parsePrimary(). - Add an overload of
matchso you can match on both kind+text:
bool Parser::match(Token::Kind kind, const std::string &txt) {
if (currentToken.kind == kind && currentToken.text == txt) {
advance();
return true;
}
return false;
}- Change
parseUnary()to handle “–” in one place. If it’s directly in front of a literal you’ll still get a single literal node; if it’s in front of any other expression you get a proper unary‐negate node:
std::unique_ptr<ASTNode> Parser::parseUnary() {
// special‐case negative literal:
if (match(Token::Kind::Symbol, "-") &&
(currentToken.kind == Token::Kind::IntegerLiteral ||
currentToken.kind == Token::Kind::FloatLiteral))
{
// capture "-" + literal text, then let primary do the rest
std::string txt = "-" + currentToken.text;
auto kind = currentToken.kind;
advance();
return (kind == Token::Kind::IntegerLiteral)
? ASTNode::makeIntegerLiteralNode(txt, currentScope)
: ASTNode::makeFloatLiteralNode (txt, currentScope);
}
// generic unary operator (e.g. negate a sub‐expr)
if (match(Token::Kind::Symbol, "-")) {
auto operand = parseUnary();
return ASTNode::makeUnaryOperatorNode(UnaryOp::Negate,
std::move(operand),
currentScope);
}
// fall back to primary
return parsePrimary();
}That eliminates the nested if(“-”) in parsePrimary and centralizes all unary‐minus handling in one place.
We never saw that coming
Summary by Sourcery
Standardize token kind references and relocate negative numeric literal handling from the lexer to the parser; extend CI workflows with improved caching and branch triggers.
New Features:
Enhancements:
CI: