Parser!! #20
Parser!! #20
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bialger
left a comment
bialger
left a comment
There was a problem hiding this comment.
All in all, titanic work has been done. It is fantastic, and I don't see any major architecture flaws. Although there is a place to grow and some simple mistakes. Please fix them. Really good job, thank you.
…ing [[nodiscard]] attributes
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const Token& VectorTokenStream::Peek(size_t k) { | ||
| const Token* token = TryPeek(k); | ||
|
|
||
| if (token != nullptr) { | ||
| return *token; | ||
| } | ||
|
|
||
| if (last_ != nullptr) { | ||
| return *last_; | ||
| } | ||
|
|
||
| if (!tokens_.empty()) { | ||
| return *tokens_.back(); | ||
| } | ||
|
|
||
| throw std::out_of_range("VectorTokenStream::Peek out of range"); | ||
| } | ||
|
|
||
| TokenPtr VectorTokenStream::Consume() { | ||
| if (index_ < tokens_.size()) { | ||
| last_ = tokens_[index_].get(); | ||
| return std::move(tokens_[index_++]); | ||
| } | ||
|
|
||
| last_ = nullptr; |
There was a problem hiding this comment.
In VectorTokenStream::Consume the stream stores a raw pointer to the consumed token (last_ = tokens_[index_].get();) and then moves the unique_ptr out to the caller. Peek and LastConsumed later return *last_ when no tokens remain. If the caller releases the returned TokenPtr—which destroys the token—the stored pointer dangles and the next call to Peek or LastConsumed dereferences freed memory. The stream should either retain ownership of consumed tokens or avoid returning pointers/references after handing ownership to the caller; otherwise calls past EOF have undefined behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive parser infrastructure for the Ovum compiler. It implements a type system with type references, nullability tracking, AST nodes for expressions/statements/declarations, visitor patterns for AST traversal, parser states, diagnostics with severity levels, and Pratt parsing infrastructure.
- Introduces
TypeReferenceclass with qualified names, type arguments, nullability, and resolution tracking - Implements full AST node hierarchy including expressions, statements, declarations, and class members
- Adds visitor pattern support with WalkVisitor, ConstWalkVisitor, PrintVisitor, StructuralValidator, and LintVisitor
- Provides diagnostic system with severity levels, error collection, and filtering capabilities
- Sets up parser state machine infrastructure and token stream handling
Reviewed Changes
Copilot reviewed 231 out of 231 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/parser/types/TypeReference.{hpp,cpp} | Core type reference implementation with qualified names, type arguments, nullability, and resolution |
| lib/parser/types/Param.{hpp,cpp} | Parameter representation with name and type |
| lib/parser/types/Nullable.{hpp,cpp} | Nullable type wrapper |
| lib/parser/ast/nodes/* | Complete AST node hierarchy for expressions, statements, declarations, and class members |
| lib/parser/ast/visitors/* | Visitor pattern implementations for AST traversal and validation |
| lib/parser/diagnostics/* | Diagnostic system with severity levels, collection, and filtering |
| lib/parser/states/* | Parser state machine infrastructure (headers only, implementations pending) |
| lib/parser/pratt/* | Pratt parser infrastructure for expression parsing |
| lib/parser/tokens/* | Token stream and source tracking utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [[nodiscard]] std::string GetName(); | ||
|
|
||
| [[nodiscard]] TypeReference GetType(); |
There was a problem hiding this comment.
The GetName() and GetType() methods should return const references for consistency with the rest of the codebase. These methods copy the entire std::string and TypeReference objects on every call, which is inefficient.
Change:
[[nodiscard]] std::string GetName();
[[nodiscard]] TypeReference GetType();To:
[[nodiscard]] const std::string& GetName() const;
[[nodiscard]] const TypeReference& GetType() const;
No description provided.