feat(tdl): Add StructSpec AST node. - #190
Conversation
WalkthroughAdds a new StructSpec AST node (header + source) to the TDL parser, wires it into spider_tdl's CMake exports and public linkage, updates Node.hpp include, and adds unit tests validating creation, duplicate/empty errors, and serialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Client/Test
participant StructSpec
participant NodeValidation as Validator
participant NamedVar
participant Identifier
Test->>StructSpec: create(name: Node, fields: vector<Node>)
StructSpec->>Identifier: validate name is Identifier
StructSpec->>NamedVar: validate each field is NamedVar
StructSpec->>NodeValidation: check duplicate names / empty list
alt invalid
StructSpec-->>Test: Error (DuplicatedFieldName | EmptyStruct | type error)
else valid
StructSpec-->>Test: shared_ptr<StructSpec>
end
Test->>StructSpec: serialize_to_str(indent)
StructSpec->>NamedVar: visit_fields(serialize each field)
StructSpec-->>Test: formatted string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-07T14:54:20.267ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/tdl/test-parser-ast.cpp (1)
262-274: Strengthen assertions for StructSpec in “Basic” sectionAdd checks for get_num_fields() and exercise visit_fields to validate iteration order. This improves coverage of the new API.
REQUIRE(nullptr != struct_spec_node); - REQUIRE(struct_spec_node->get_num_children() == 4); + REQUIRE(struct_spec_node->get_num_children() == 4); + REQUIRE(struct_spec_node->get_num_fields() == 3); + // Optionally, verify visit_fields iterates all fields in order. + size_t seen = 0; + auto visit_result = struct_spec_node->visit_fields([&](NamedVar const& nv) + -> Result<void> { + // Sanity check that field Id exists; order is implied by visit order. + REQUIRE(nv.get_id() != nullptr); + ++seen; + return ystdlib::error_handling::success(); + }); + REQUIRE_FALSE(visit_result.has_error()); + REQUIRE(seen == 3);src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
52-61: Reserve capacity for duplicate-check setA small perf win: reserve to avoid rehashing when there are many fields.
- std::unordered_set<std::string_view> field_names; + std::unordered_set<std::string_view> field_names; + field_names.reserve(fields.size());
71-96: Reserve serialisation buffer and minor micro-optimisationPre-sizing the vector avoids reallocations, especially for larger structs. No behaviour change.
- std::vector<std::string> serialized_fields; + std::vector<std::string> serialized_fields; + serialized_fields.reserve(get_num_fields());src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
58-59: Optional: mark simple accessors noexceptget_num_fields() and get_name() are non-throwing given factory invariants. Marking them noexcept can help callers and tooling.
- [[nodiscard]] auto get_name() const -> std::string_view { + [[nodiscard]] auto get_name() const noexcept -> std::string_view { ... - [[nodiscard]] auto get_num_fields() const -> size_t { return get_num_children() - 1; } + [[nodiscard]] auto get_num_fields() const noexcept -> size_t { return get_num_children() - 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/spider/CMakeLists.txt(2 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
name(26-28)name(26-26)
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (4)
StructSpec(87-87)name(44-45)indentation_level(48-49)visit_fields(72-73)src/spider/tdl/parser/ast/Node.hpp (1)
child(103-104)
tests/tdl/test-parser-ast.cpp (4)
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
StructSpec(87-87)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
create(44-69)create(44-45)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(17-26)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
create(69-84)create(69-70)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (5)
src/spider/CMakeLists.txt (1)
202-203: Wiring StructSpec into spider_tdl looks correctStructSpec.cpp/StructSpec.hpp are properly added alongside Identifier/NamedVar. Public visibility via target_sources(spider_tdl PUBLIC …) is consistent. No build-system concerns.
Also applies to: 219-219
tests/tdl/test-parser-ast.cpp (2)
306-322: Duplicate-field-name error-path test looks solidAsserts the intended DuplicatedFieldName error. The SECTION note about moved fields is correct for Catch2’s branching model.
324-331: Empty-struct error-path test is appropriateCorrectly validates EmptyStruct. Nice coverage for both major error cases.
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (1)
43-69: Use of string_view for duplicate detection is safe hereYou collect names before moving fields into the StructSpec, so the string_views remain valid during the check. The approach is sound.
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
67-83: visit_fields API is clean and consistentConstraint on FieldVisitor and error propagation via Result align with existing patterns. Implementation looks correct.
2 participants
Description
This PR introduces
StructSpecAST node, which represents the specification of a struct defined in TDL. A struct should have an ID (Identifier) and a list of fields (NamedVar), where the fields should be uniquely identified by the name.Notice that despite
StructSpecis implemented as an AST node, it is not aTypeobject. The actual struct type should have a reference to such aStructSpecobject, as the specification might be shared by multiple uses of a struct type. Thus, the factory function ofStructSpecreturns a shared pointer for reference sharing, instead of a unique pointer.Checklist
breaking change.
Validation performed
StructSpec.Summary by CodeRabbit
New Features
Tests
Chores