Skip to content

feat(tdl): Add Type AST nodes for primitive and container types. - #184

Merged
LinZhihao-723 merged 24 commits into
y-scope:mainfrom
LinZhihao-723:ast-type
Aug 8, 2025
Merged

feat(tdl): Add Type AST nodes for primitive and container types.#184
LinZhihao-723 merged 24 commits into
y-scope:mainfrom
LinZhihao-723:ast-type

Conversation

@LinZhihao-723

@LinZhihao-723 LinZhihao-723 commented Aug 7, 2025

Copy link
Copy Markdown
Member

Description

Reference

This PR depends on #183.

Overview

This PR adds Type AST nodes for primitive and container types, following this hierarchy:

flowchart LR
Node --> Type["class Type"]
Type --> Primitive["class Primitive"]
Primitive --> Int["class Int"]
Primitive --> Float["class Float"]
Primitive --> Boolean["class Boolean"]
Type --> Container["class Container"]
Container --> List["class List"]
Container --> Map["class Map"]
Loading

Notice that we still need to support Struct after we introduce NamedVar.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Ensure all workflows pass.
  • Add unit tests to check the basic behaviors.

Summary by CodeRabbit

New Features

  • Introduced support for new AST node types, including primitive types (Bool, Int, Float) and container types (List, Map).
  • Added detailed error handling and serialization for these node types.
  • Implemented utility functions for type specification and node validation.

Bug Fixes

  • Improved error messages for unexpected node types and unknown type specifications.

Tests

  • Expanded test coverage to include all new primitive and container node types, their serialization, and error scenarios.

Chores

  • Updated build configuration to include new source files and dependencies.

@LinZhihao-723
LinZhihao-723 requested review from a team and sitaowang1998 as code owners August 7, 2025 22:09
@coderabbitai

coderabbitai Bot commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This update introduces a comprehensive set of new AST node types and supporting utilities for the TDL parser, including primitive types (Bool, Int, Float), container types (List, Map), type specification enums, and serialization/validation helpers. Build configuration and tests are expanded to cover these new AST node classes and their behaviours.

Changes

Cohort / File(s) Change Summary
Build System Updates
src/spider/CMakeLists.txt
Expanded build configuration to include new AST node, type spec, and utility source/header files. Updated spider_tdl target to link with fmt::fmt and ystdlib::error_handling.
Type Specification Enums
src/spider/tdl/parser/ast/FloatSpec.hpp, src/spider/tdl/parser/ast/IntSpec.hpp
Added FloatSpec and IntSpec enum classes for floating-point and integer type specifications, respectively.
AST Node Base and Error Handling
src/spider/tdl/parser/ast/Node.cpp, src/spider/tdl/parser/ast/Node.hpp
Added new error codes to Node and error message mapping; introduced a protected default constructor for Node.
AST Node Hierarchy: Base Classes
src/spider/tdl/parser/ast/node_impl/Type.hpp, src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp, src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp
Introduced abstract base classes for Type, Container, and Primitive AST node types.
Primitive Type Nodes
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp, src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp, .../Float.cpp, .../Float.hpp, .../Int.cpp, .../Int.hpp
Implemented Bool, Float, and Int AST node classes with factory methods, serialization, and type spec accessors.
Container Type Nodes
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp, .../List.hpp, .../Map.cpp, .../Map.hpp
Implemented List and Map container AST node classes with factory methods, serialization, error handling, and validation of child node types.
AST Utilities
src/spider/tdl/parser/ast/utils.cpp, src/spider/tdl/parser/ast/utils.hpp
Added utility functions for indentation, type spec serialization, and child node type validation.
Tests
tests/tdl/test-parser-ast.cpp
Added extensive tests for primitive and container node creation, serialization, error handling, and validation of invalid/unsupported node types.

Sequence Diagram(s)

sequenceDiagram
  participant TestSuite
  participant ASTFactory
  participant Node
  participant Type/Primitive/Container
  participant Utils

  TestSuite->>ASTFactory: create Int/Float/Bool/List/Map node(s)
  ASTFactory->>Node: validate child node type(s)
  Node-->>ASTFactory: success or error
  ASTFactory->>Type/Primitive/Container: instantiate node
  Type/Primitive/Container-->>ASTFactory: node instance
  TestSuite->>Type/Primitive/Container: serialize_to_str()
  Type/Primitive/Container->>Utils: create_indentation(), serialize_int_spec/float_spec()
  Utils-->>Type/Primitive/Container: formatted string or error
  Type/Primitive/Container-->>TestSuite: serialized string or error
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

Possibly related PRs

Note

🔌 MCP (Model Context Protocol) integration is now available in Early Access!

Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6283a5 and 1bfaf5a.

📒 Files selected for processing (3)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (1 hunks)
  • tests/tdl/test-parser-ast.cpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/tdl/test-parser-ast.cpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: lint
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (1)

44-54: Consider adding noexcept specifier for consistency.

The accessor methods are correctly implemented with appropriate static_cast usage and documentation. Consider adding noexcept specifiers for consistency with similar accessor methods in other container types like List.

-    [[nodiscard]] auto get_key_type() const -> Type const* {
+    [[nodiscard]] auto get_key_type() const noexcept -> Type const* {
        // The factory function ensures that the first child is of type `Type`.
        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast)
        return static_cast<Type const*>(get_child_unsafe(0));
    }

-    [[nodiscard]] auto get_value_type() const -> Type const* {
+    [[nodiscard]] auto get_value_type() const noexcept -> Type const* {
        // The factory function ensures that the first child is of type `Type`.
        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast)
        return static_cast<Type const*>(get_child_unsafe(1));
    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8689d5 and 4cb87dc.

📒 Files selected for processing (23)
  • src/spider/CMakeLists.txt (2 hunks)
  • src/spider/tdl/parser/ast/FloatSpec.hpp (1 hunks)
  • src/spider/tdl/parser/ast/IntSpec.hpp (1 hunks)
  • src/spider/tdl/parser/ast/Node.cpp (1 hunks)
  • src/spider/tdl/parser/ast/Node.hpp (2 hunks)
  • src/spider/tdl/parser/ast/node_impl/Identifier.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/Type.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (1 hunks)
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (1 hunks)
  • src/spider/tdl/parser/ast/utils.cpp (1 hunks)
  • src/spider/tdl/parser/ast/utils.hpp (1 hunks)
  • tests/tdl/test-parser-ast.cpp (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: LinZhihao-723
PR: y-scope/spider#182
File: src/spider/tdl/parser/ast/Node.hpp:0-0
Timestamp: 2025-08-07T14:54:20.253Z
Learning: In the spider TDL AST Node class, derived classes should use the `add_child` method in their factory functions rather than passing children directly to a constructor, to ensure proper parent-child relationship management and error handling.
📚 Learning: 2025-08-07T14:54:20.253Z
Learnt from: LinZhihao-723
PR: y-scope/spider#182
File: src/spider/tdl/parser/ast/Node.hpp:0-0
Timestamp: 2025-08-07T14:54:20.253Z
Learning: In the spider TDL AST Node class, derived classes should use the `add_child` method in their factory functions rather than passing children directly to a constructor, to ensure proper parent-child relationship management and error handling.

Applied to files:

  • src/spider/tdl/parser/ast/Node.cpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp
  • src/spider/tdl/parser/ast/node_impl/Identifier.hpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp
  • src/spider/tdl/parser/ast/node_impl/Type.hpp
  • src/spider/CMakeLists.txt
  • src/spider/tdl/parser/ast/Node.hpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp
  • src/spider/tdl/parser/ast/utils.hpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp
  • src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp
📚 Learning: 2025-07-17T19:44:06.132Z
Learnt from: sitaowang1998
PR: y-scope/spider#168
File: lint-tasks.yaml:62-65
Timestamp: 2025-07-17T19:44:06.132Z
Learning: ANTLR-generated C++ code in the G_SRC_DSL_DIR (src/stdl) should not be included in linting tasks because it's auto-generated code that doesn't follow manual coding standards.

Applied to files:

  • src/spider/CMakeLists.txt
  • src/spider/tdl/parser/ast/utils.cpp
📚 Learning: 2025-04-09T17:15:24.552Z
Learnt from: davidlion
PR: y-scope/spider#100
File: src/spider/worker/worker.cpp:205-230
Timestamp: 2025-04-09T17:15:24.552Z
Learning: Documentation should be added to new functions in the spider codebase, as already discussed with the user.

Applied to files:

  • src/spider/CMakeLists.txt
📚 Learning: 2025-04-08T05:46:35.492Z
Learnt from: davidlion
PR: y-scope/spider#100
File: src/spider/worker/worker.cpp:191-196
Timestamp: 2025-04-08T05:46:35.492Z
Learning: According to the project's coding guidelines, file-scope functions (those in unnamed namespaces) should have separate declarations and definitions.

Applied to files:

  • src/spider/tdl/parser/ast/utils.cpp
🧬 Code Graph Analysis (11)
src/spider/tdl/parser/ast/FloatSpec.hpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (2)
  • Float (35-35)
  • Float (35-35)
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/Identifier.cpp (4)
src/spider/tdl/parser/ast/Node.hpp (1)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/utils.cpp (2)
  • create_indentation (14-19)
  • create_indentation (14-14)
src/spider/tdl/parser/ast/utils.hpp (1)
  • create_indentation (21-21)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (3)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
  • nodiscard (20-22)
  • indentation_level (25-26)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (4)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • spec (22-22)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (7)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (2)
  • serialize_to_str (12-19)
  • serialize_to_str (12-13)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
  • serialize_to_str (25-33)
  • serialize_to_str (25-26)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
  • serialize_to_str (84-94)
  • serialize_to_str (84-85)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (2)
  • serialize_to_str (12-19)
  • serialize_to_str (12-13)
src/spider/tdl/parser/ast/Node.hpp (1)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
  • indentation_level (25-26)
src/spider/tdl/parser/ast/utils.cpp (2)
  • create_indentation (14-19)
  • create_indentation (14-14)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (3)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (3)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/utils.hpp (4)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (4)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
  • spec (22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (4)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
  • spec (22-22)
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (2)
  • nodiscard (31-31)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (5)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (2)
  • serialize_to_str (12-15)
  • serialize_to_str (12-13)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (2)
  • serialize_to_str (12-19)
  • serialize_to_str (12-13)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (1)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/utils.cpp (4)
  • create_indentation (14-19)
  • create_indentation (14-14)
  • serialize_float_spec (36-45)
  • serialize_float_spec (36-36)
src/spider/tdl/parser/ast/utils.hpp (2)
  • create_indentation (21-21)
  • serialize_float_spec (40-41)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
  • nodiscard (20-22)
  • indentation_level (25-26)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (4)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • spec (22-22)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (5)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (2)
  • nodiscard (32-36)
  • indentation_level (28-29)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
  • nodiscard (20-22)
  • indentation_level (25-26)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (3)
  • nodiscard (22-24)
  • nodiscard (31-31)
  • indentation_level (27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (2)
src/spider/tdl/parser/ast/Node.hpp (5)
  • nodiscard (45-45)
  • nodiscard (50-50)
  • nodiscard (73-79)
  • nodiscard (112-114)
  • indentation_level (88-89)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (3)
  • nodiscard (44-48)
  • nodiscard (50-54)
  • indentation_level (40-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: lint
🔇 Additional comments (46)
src/spider/tdl/parser/ast/Node.cpp (1)

28-31: LGTM!

The new error codes and their corresponding descriptive messages are well-defined and consistent with the existing error handling pattern. The messages clearly communicate the nature of each error condition.

src/spider/tdl/parser/ast/FloatSpec.hpp (1)

1-15: LGTM!

The FloatSpec enum is well-designed with proper scoping, explicit underlying type, and clear naming. The include guards and namespace structure follow the project conventions correctly.

src/spider/tdl/parser/ast/Node.hpp (2)

24-25: LGTM!

The new error codes are appropriately added to support the extended AST node functionality for type validation and specification handling.


93-94: LGTM!

The protected default constructor is correctly implemented for the base class. This follows good inheritance design principles where the base class constructor is accessible to derived classes but not to external code.

src/spider/tdl/parser/ast/node_impl/Type.hpp (1)

1-12: LGTM!

The Type abstract base class is correctly implemented as a foundation for the type hierarchy. The simple design with public inheritance from Node is appropriate for this use case.

src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp (1)

1-12: LGTM!

The Container abstract base class correctly extends the type hierarchy from Type. The implementation follows the established pattern and supports the intended class structure for container types like List and Map.

src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)

6-9: LGTM! Clean abstract base class definition.

The Primitive class provides a proper foundation for all primitive type nodes in the AST hierarchy. The implementation follows the established pattern with appropriate namespace organization and inheritance structure.

src/spider/tdl/parser/ast/IntSpec.hpp (1)

6-14: LGTM! Well-designed enum class for integer specifications.

The IntSpec enum class follows good practices with scoped enumeration, compact uint8_t underlying type, and clear naming for standard integer sizes. This provides a solid foundation for the integer primitive type nodes.

src/spider/tdl/parser/ast/node_impl/Identifier.cpp (1)

12-15: LGTM! Consistent serialization implementation.

The serialize_to_str method follows the established pattern used across other AST nodes, with proper indentation handling, error wrapping, and clean string formatting using fmt::format.

src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (1)

12-15: LGTM! Appropriate serialization for Bool primitive type.

The serialize_to_str implementation correctly follows the established pattern. The simpler format without specification details (unlike Int and Float types) is appropriate since Bool doesn't have type variants requiring additional specification information.

src/spider/CMakeLists.txt (3)

198-209: LGTM! Comprehensive source file additions for AST nodes.

The new source files are well-organized following the established directory hierarchy and cover all the AST node implementations (identifiers, primitive types, container types, and utilities).


211-227: LGTM! Complete header file organization for AST type system.

The header files are systematically organized, covering all specification enums, base classes, and concrete implementations. The structure clearly reflects the type hierarchy design.


234-239: LGTM! Appropriate public linkage of fmt library.

Adding fmt::fmt as a PUBLIC dependency is correct since the AST node headers use fmt formatting in their implementations, requiring downstream consumers to have access to the fmt library.

src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (2)

1-10: LGTM - Includes are appropriate and well-organized.

The includes are properly organized with system headers first, followed by third-party libraries (fmt, ystdlib), and finally project-specific headers. All necessary dependencies for the implementation are included.


12-19: LGTM - Consistent implementation following established patterns.

The serialize_to_str method implementation correctly follows the same pattern as other primitive types (Int, Bool). It properly uses:

  • create_indentation for formatting consistency
  • serialize_float_spec with error handling via YSTDLIB_ERROR_HANDLING_TRYX
  • fmt::format for string construction
  • Consistent output format [Type[Primitive[Float]]]:{spec}
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (4)

1-12: LGTM - Proper header structure and includes.

Header guards follow the correct naming convention, and all necessary includes are present for the class definition and its dependencies.


20-22: LGTM - Factory method follows established pattern.

The factory method correctly returns a std::unique_ptr<Node> and uses std::make_unique with brace initialization, consistent with other primitive type implementations (Int, Float). The parameterless design is appropriate since Bool has no specification variants.


25-26: LGTM - Proper method signature override.

The serialize_to_str method correctly overrides the base class virtual method with the same signature and return type.


30-30: LGTM - Appropriate constructor design.

The private, defaulted constructor enforces the factory pattern and is appropriate since Bool requires no initialization parameters.

src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (2)

1-10: LGTM - Consistent include organization.

The includes follow the same organizational pattern as Float.cpp with proper separation of system, third-party, and project headers.


12-19: LGTM - Correct implementation following established pattern.

The serialize_to_str method implementation is consistent with Float.cpp, using:

  • create_indentation for proper formatting
  • serialize_int_spec(m_spec) with error propagation
  • Consistent output format [Type[Primitive[Int]]]:{spec}
  • Proper error handling with YSTDLIB_ERROR_HANDLING_TRYX
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (4)

1-13: LGTM - Proper header structure and necessary includes.

Header guards follow the correct naming convention. All necessary includes are present, including <utility> for std::move and <string_view> for the getter method.


22-24: LGTM - Well-designed factory method with move semantics.

The factory method correctly:

  • Takes the name parameter by value for optimal move semantics
  • Uses std::move when passing to the constructor
  • Returns std::unique_ptr<Node> consistent with other factory methods
  • Uses brace initialization with std::make_unique

31-31: LGTM - Efficient getter method.

The get_name method returns std::string_view which avoids unnecessary copying and provides const access to the stored name.


35-35: LGTM - Proper constructor with move semantics.

The private constructor correctly uses std::move to efficiently transfer the string parameter to the member variable, and is marked noexcept appropriately.

src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (3)

1-14: LGTM - Comprehensive includes for container implementation.

All necessary headers are included for the implementation, including validation utilities, error handling, and formatting dependencies.


16-23: LGTM - Correct implementation following established learning.

The factory method correctly follows the retrieved learning by:

  • Using add_child method instead of passing children to constructor (line 21)
  • Validating child node type using validate_child_node_type<Type> (line 18)
  • Proper error handling with YSTDLIB_ERROR_HANDLING_TRYV
  • Creating the node first, then adding the child to establish parent-child relationships properly

This aligns with the learning that "derived classes should use the add_child method in their factory functions rather than passing children directly to a constructor."


25-33: LGTM - Well-formatted serialization with proper indentation.

The serialize_to_str method implementation correctly:

  • Uses multi-line formatting for better readability
  • Applies appropriate indentation levels (+1 for "ElementType:", +2 for the actual element type)
  • Recursively serializes the element type with proper error propagation
  • Maintains consistent formatting style with other container types
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (1)

14-40: LGTM!

The Int class implementation follows good design patterns with a private constructor and static factory method. The class structure is clean and well-organized.

src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (1)

14-40: LGTM!

The Float class implementation is consistent with the Int class design, maintaining the same factory pattern and structure. Well implemented.

src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1)

14-41: LGTM!

The List class header is well-designed with proper error handling through Result return type in the factory method. The get_element_type method correctly relies on factory validation with appropriate documentation.

src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)

69-82: LGTM! Factory method correctly uses add_child.

The factory implementation properly validates child node types and uses the add_child method as per the established pattern, ensuring proper parent-child relationship management.

src/spider/tdl/parser/ast/utils.hpp (1)

43-62: Well-designed template validation function.

The validate_child_node_type template function is well-implemented with proper SFINAE constraint using requires clause and appropriate error handling using dynamic_cast for runtime type checking.

tests/tdl/test-parser-ast.cpp (7)

3-35: LGTM: Comprehensive test setup.

The includes and using declarations properly support the extensive testing of the new AST node types. Good use of Catch2 generators for parameterized testing.


37-51: LGTM: Thorough Identifier node testing.

Comprehensive coverage of Identifier node creation, properties, and serialization with proper assertions and test data.


53-73: LGTM: Well-designed parameterized testing.

Excellent use of Catch2 generators to test all IntSpec values comprehensively. The dynamic construction of expected serialization results ensures consistency with the utility functions.


75-94: LGTM: Consistent parameterized testing approach.

Good consistency with the Int test pattern, providing comprehensive coverage of FloatSpec values with proper assertions.


96-105: LGTM: Appropriate Bool node testing.

Simple and effective testing for the Bool node, which correctly doesn't need parameterization unlike Int and Float types.


162-182: LGTM: Comprehensive error handling validation.

Excellent negative testing that verifies proper error codes are returned for invalid container type creation scenarios.


184-202: LGTM: Thorough Map key type validation testing.

Good coverage of Map-specific error handling for unsupported key types with clear commenting on the testing strategy.

src/spider/tdl/parser/ast/utils.cpp (3)

14-19: LGTM: Well-implemented indentation utility.

The function correctly creates indentation with appropriate documentation explaining the braced init list limitation and proper NOLINT usage.


21-34: LGTM: Correct IntSpec serialization with proper error handling.

The function correctly maps all IntSpec enum values to their string representations with appropriate error handling for unknown values.


36-45: LGTM: Consistent FloatSpec serialization implementation.

Good consistency with the IntSpec serialization function, properly handling all FloatSpec values with appropriate error handling.

src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (3)

17-25: LGTM: Well-designed error handling.

Proper class hierarchy with Container inheritance and well-structured error code enum using the project's error handling conventions.


26-37: LGTM: Well-designed factory function with comprehensive documentation.

The factory function follows best practices with proper parameter types, return type, and thorough documentation of error conditions and behavior.


56-66: LGTM: Proper encapsulation and error code integration.

Good use of private constructor to enforce factory usage and proper integration with the error handling system using the required macro.

Comment thread src/spider/tdl/parser/ast/utils.hpp Outdated
Comment thread tests/tdl/test-parser-ast.cpp
Comment thread tests/tdl/test-parser-ast.cpp
Comment thread src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp Outdated
Comment thread tests/tdl/test-parser-ast.cpp Outdated
Comment thread tests/tdl/test-parser-ast.cpp Outdated
@LinZhihao-723
LinZhihao-723 merged commit 4ad53af into y-scope:main Aug 8, 2025
5 checks passed
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