feat(tdl): Add Struct AST node. - #193
Conversation
WalkthroughAdds a new TDL AST node type Struct (header + implementation), registers it in the public build, implements creation, serialization and spec-assignment with explicit error codes and category, and extends parser AST tests to cover creation, naming, serialization, spec assignment, duplicate-spec and mismatched-spec error paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Identifier
participant Struct
participant StructSpec
Test->>Identifier: Identifier::create("TestStruct")
Test->>Struct: Struct::create(Identifier)
Struct-->>Test: Result<unique_ptr<Struct>>
Test->>StructSpec: StructSpec::create("TestStruct", fields)
Test->>Struct: Struct::set_spec(StructSpec)
Struct-->>Test: Result<void> (ok or ErrorCode)
Test->>Struct: Struct::serialize_to_str(indent)
Struct-->>Test: Result<string>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 2
🧹 Nitpick comments (5)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (2)
50-59: Document the null-spec error and keep the contract explicit.Reflect the new error in the API contract of set_spec to prevent misuse.
/** * Sets the specification for this struct. * @param spec * @return A void result on success, or an error code indicating the failure: * - ErrorCodeEnum::StructSpecNameMismatch if `spec`'s name does not match the underlying name. * - ErrorCodeEnum::StructSpecAlreadySet if the specification has already been set. + * - ErrorCodeEnum::NullStructSpec if `spec` is nullptr. */ [[nodiscard]] auto set_spec(std::shared_ptr<StructSpec> spec) -> ystdlib::error_handling::Result<void>;
44-49: get_name() assumes the factory invariant; consider a debug assertion.Not a blocker, but adding a debug-time assertion that the child is an Identifier makes violations easier to catch during development.
[[nodiscard]] auto get_name() const -> std::string_view { - // The factory function ensures that the first child is of type `Identifier`. + // The factory function ensures that the first child is of type `Identifier`. + // Consider guarding this in debug builds if available: + // YSTDLIB_ASSERT(dynamic_cast<Identifier const*>(get_child_unsafe(0)) != nullptr); // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast) return static_cast<Identifier const*>(get_child_unsafe(0))->get_name(); }src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (1)
16-34: Prefer specialising the primary template directly rather than via a type alias.Some compilers/tools can be finicky with explicit specializations referred to through aliases. Specialising ystdlib::error_handling::ErrorCategoryStruct::ErrorCodeEnum directly avoids ambiguity. Also, add the message for the new NullStructSpec code.
-using StructErrorCodeCategory = ystdlib::error_handling::ErrorCategory<Struct::ErrorCodeEnum>; - -template <> -auto StructErrorCodeCategory::name() const noexcept -> char const* { +template <> +auto ystdlib::error_handling::ErrorCategory<Struct::ErrorCodeEnum>::name() const noexcept + -> char const* { return "spider::tdl::parser::ast::node_impl::type_impl::Struct"; } template <> -auto StructErrorCodeCategory::message(Struct::ErrorCodeEnum error_enum) const -> std::string { +auto ystdlib::error_handling::ErrorCategory<Struct::ErrorCodeEnum>::message( + Struct::ErrorCodeEnum error_enum) const -> std::string { switch (error_enum) { case Struct::ErrorCodeEnum::StructSpecAlreadySet: return "The struct spec is already set."; case Struct::ErrorCodeEnum::StructSpecNameMismatch: return "The struct spec name does not match the type name."; + case Struct::ErrorCodeEnum::NullStructSpec: + return "The struct spec is null."; default: return "Unknown error code enum"; } }tests/tdl/test-parser-ast.cpp (2)
363-376: Also assert get_spec() after setting the spec to catch regressions.After the successful set_spec, verify get_spec() is non-null and (optionally) points to the same object.
REQUIRE_FALSE(struct_node->set_spec(struct_spec_result.value()).has_error()); +REQUIRE(struct_node->get_spec() != nullptr); +// Optional: if struct_spec_result.value() is a shared_ptr<StructSpec>, compare addresses. +REQUIRE(struct_node->get_spec() == struct_spec_node);Note: If comparing addresses, ensure struct_spec_node remains valid and refers to the same underlying object.
386-395: Good negative test; consider adding an invalid-child-type case for Struct::create.You’ve covered name mismatch and duplicate assignment. It’d be valuable to assert that Struct::create rejects a non-Identifier child and propagates Node::UnexpectedChildNodeType (mirroring your container tests).
For example, add:
SECTION("Invalid input for Struct creation") { auto wrong_name{Int::create(IntSpec::Int64)}; auto struct_result{Struct::create(std::move(wrong_name))}; REQUIRE(struct_result.has_error()); REQUIRE(struct_result.error() == Node::ErrorCode{Node::ErrorCodeEnum::UnexpectedChildNodeType}); }
📜 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/type_impl/Struct.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(4 hunks)
⏰ 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (8)
src/spider/CMakeLists.txt (2)
198-214: Struct node is properly wired into the TDL target.Struct.cpp is added to SPIDER_TDL_SHARED_SOURCES and will be compiled into spider_tdl as expected.
215-236: Public header exposure looks correct.Struct.hpp is exported via SPIDER_TDL_SHARED_HEADERS and target_sources(spider_tdl PUBLIC ...), making it available to dependants. If you have install/export rules elsewhere, ensure this header is included there as well.
Would you like me to scan the repository for install/export rules to ensure Struct.hpp is included where needed?
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
46-58: LGTM: Serialization is consistent with the test expectation and existing style.Indentation and child serialization match "[Type[Struct]]" with a nested Identifier.
37-44: No change needed — validate_child_node_type handles nullptr and the make_unique(T{}) usage is consistentvalidate_child_node_type (src/spider/tdl/parser/ast/utils.hpp) uses dynamic_cast and returns Node::ErrorCodeEnum::UnexpectedChildNodeType for nullptr, so no extra null check is required. The make_unique(T{}) pattern is used consistently across the codebase (e.g. NamedVar.cpp, StructSpec.cpp, Tuple.cpp, Map.cpp, List.cpp, Struct.cpp), so changing this one factory would be inconsistent; if you want to avoid constructing via the move/copy ctor, do a separate, repository-wide refactor.
Files inspected:
- src/spider/tdl/parser/ast/utils.hpp (validate_child_node_type)
- src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp
- src/spider/tdl/parser/ast/node_impl/NamedVar.cpp
- src/spider/tdl/parser/ast/node_impl/StructSpec.cpp
- src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp
- src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp
- src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp
tests/tdl/test-parser-ast.cpp (4)
25-26: Include and alias of Struct are correct.Public header path and namespace aliasing match the new API surface.
331-361: StructSpec serialization test is thorough and stable.Covers all fields and matches the expected tree shape. No issues.
377-385: Struct serialization test matches the implementation.The expected string aligns with Struct::serialize_to_str.
317-329: Nice coverage for StructSpec properties.Validates child count and name; pairs well with the serialization checks.
| auto const duplicated_set_spec{struct_node->set_spec(struct_spec_result.value())}; | ||
| REQUIRE(duplicated_set_spec.has_error()); | ||
| REQUIRE(duplicated_set_spec.error() | ||
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}); |
There was a problem hiding this comment.
Duplicate test should be put into a separate section.
| auto const duplicated_set_spec{struct_node->set_spec(struct_spec_result.value())}; | |
| REQUIRE(duplicated_set_spec.has_error()); | |
| REQUIRE(duplicated_set_spec.error() | |
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}); | |
| SECTION("Set spec twice.") | |
| auto const duplicated_set_spec{struct_node->set_spec(struct_spec_result.value())}; | |
| REQUIRE(duplicated_set_spec.has_error()); | |
| REQUIRE(duplicated_set_spec.error() | |
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}); | |
| } |
There was a problem hiding this comment.
I don't think this is necessary: adding one more level of SECTION will enforce the control flow before this section to run twice iiuc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/tdl/test-parser-ast.cpp (4)
401-407: Avoid redundant dynamic_cast on StructSpec shared_ptrThe dynamic_cast here is unnecessary since you already checked the Result has no error and you’re holding a shared_ptr. A simple non-null check communicates intent better.
Apply this diff:
- REQUIRE(nullptr != dynamic_cast<StructSpec const*>(struct_spec_result.value().get())); + REQUIRE(struct_spec_result.value() != nullptr);
408-434: Add a post-set_spec serialization assertionGiven Struct serialization currently prints only the name, it’s valuable to assert serialization remains unchanged after spec assignment. This prevents regressions if future changes inadvertently alter Struct’s serialization when a spec is set.
Apply this diff right after verifying get_spec() is non-null:
// Set the `StructSpec` to the `Struct` REQUIRE_FALSE(struct_node->set_spec(struct_spec_result.value()).has_error()); REQUIRE(nullptr != struct_node->get_spec()); + + // Serialization should be unaffected by spec assignment + auto const serialized_after_set{struct_node->serialize_to_str(0)}; + REQUIRE_FALSE(serialized_after_set.has_error()); + REQUIRE(serialized_after_set.value() == cExpectedSerializedResult);
436-445: Ensure no partial state on failed set_spec (spec remains null)After the name-mismatch error, also assert that the Struct’s spec remains unset to guard against partial updates.
Apply this diff after the error assertion:
REQUIRE(set_spec_result.has_error()); REQUIRE(set_spec_result.error() == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); + // Ensure spec is still null after a failed set_spec + REQUIRE(nullptr == struct_node->get_spec());
390-446: Add negative test: Struct::create should reject non-Identifier nameSince Struct::create validates the child node type (Identifier expected), add a test ensuring incorrect child types (e.g., NamedVar) yield UnexpectedChildNodeType. This mirrors existing negative coverage for containers and hardens the contract.
Apply this diff to add a sibling SECTION under "Struct", right after creating struct_spec_result:
REQUIRE_FALSE(struct_spec_result.has_error()); - REQUIRE(nullptr != dynamic_cast<StructSpec const*>(struct_spec_result.value().get())); + REQUIRE(struct_spec_result.value() != nullptr); + SECTION("Invalid name type") { + auto named_var_result{ + NamedVar::create(Identifier::create("m_x"), Int::create(IntSpec::Int64)) + }; + REQUIRE_FALSE(named_var_result.has_error()); + auto bad_struct_result{Struct::create(std::move(named_var_result.value()))}; + REQUIRE(bad_struct_result.has_error()); + REQUIRE(bad_struct_result.error() + == Node::ErrorCode{Node::ErrorCodeEnum::UnexpectedChildNodeType}); + } + SECTION("Struct with StructSpec") {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/tdl/test-parser-ast.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tdl/test-parser-ast.cpp (3)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
Struct(64-64)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
create(37-44)create(37-38)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(17-26)create(17-18)
⏰ 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (2)
tests/tdl/test-parser-ast.cpp (2)
25-25: Correct header inclusion for new Struct typeIncluding Struct.hpp is necessary and correct for the new tests. No issues.
41-41: Good alias for brevity and readabilityThe using-alias for Struct reduces verbosity and keeps the test readable. Looks good.
LinZhihao-723
changed the title
Struct AST node. (#190)Struct AST node.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/tdl/test-parser-ast.cpp (2)
436-440: Optional: Move duplicate-set-spec check into its own SECTION for isolation.
Keeping it separate avoids reusing state mutated by earlier steps and clarifies intent. That said, Catch2 re-runs parent scopes for each SECTION, so your current approach is still correct.
408-440: Strengthen assertions: verify spec identity and serialisation remains unchanged after spec is set.Confirmed: Struct::get_spec() returns StructSpec const* (m_spec is std::shared_ptr), so compare to struct_spec_result.value().get().
- File to update: tests/tdl/test-parser-ast.cpp — SECTION("Struct with StructSpec") (around lines ~408–440)
Apply this diff:
// Set the `StructSpec` to the `Struct` REQUIRE_FALSE(struct_node->set_spec(struct_spec_result.value()).has_error()); - REQUIRE(nullptr != struct_node->get_spec()); + REQUIRE(nullptr != struct_node->get_spec()); + // Ensure the exact spec instance is stored. + REQUIRE(struct_node->get_spec() == struct_spec_result.value().get()); + + // Serialisation should not include StructSpec and thus remain unchanged. + auto const serialized_after_set{struct_node->serialize_to_str(0)}; + REQUIRE_FALSE(serialized_after_set.has_error()); + REQUIRE(serialized_after_set.value() == cExpectedSerializedResult);
🧹 Nitpick comments (4)
tests/tdl/test-parser-ast.cpp (2)
393-407: Factor out StructSpec construction to a helper to avoid state sharing across SECTIONs.
Although Catch2 SECTION execution model makes this safe, a small builder eliminates reliance on that nuance and reduces duplication.Consider extracting a helper:
auto make_test_struct_spec(std::string name = "TestStruct") -> std::shared_ptr<StructSpec> { auto int_field = NamedVar::create(Identifier::create("m_int"), Int::create(IntSpec::Int64)); REQUIRE_FALSE(int_field.has_error()); std::vector<std::unique_ptr<Node>> fields; fields.emplace_back(std::move(int_field.value())); auto spec = StructSpec::create(Identifier::create(std::move(name)), std::move(fields)); REQUIRE_FALSE(spec.has_error()); return spec.value(); }Then use
auto struct_spec = make_test_struct_spec(std::string{cTestStructName});in each SECTION.
406-407: Redundant dynamic_cast on a known type.
StructSpec::create(...)returns astd::shared_ptr<StructSpec>, sodynamic_cast<StructSpec const*>is unnecessary here.Apply this diff:
- REQUIRE(nullptr != dynamic_cast<StructSpec const*>(struct_spec_result.value().get())); + REQUIRE(struct_spec_result.value() != nullptr);src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
20-37: Nit: Harmonise error messages and wording.
- Consider using “null” instead of “NULL” for consistency with typical wording.
- Optionally, make the default message more specific, e.g., “Unknown Struct::ErrorCodeEnum”.
Apply this diff:
- return "The struct spec is NULL."; + return "The struct spec is null."; - return "Unknown error code enum"; + return "Unknown Struct::ErrorCodeEnum";
63-78: set_spec covers all error paths and avoids null dereference.
Order of checks favours “already set” over “null arg”; if the intended contract is to validate inputs first regardless of state, consider swapping the first two checks. Otherwise, this is fine and well-tested.Optional reordering for input-first validation:
-auto Struct::set_spec(std::shared_ptr<StructSpec> spec) -> ystdlib::error_handling::Result<void> { - if (nullptr != m_spec) { - return Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}; - } - - if (nullptr == spec) { - return Struct::ErrorCode{Struct::ErrorCodeEnum::NullStructSpec}; - } +auto Struct::set_spec(std::shared_ptr<StructSpec> spec) -> ystdlib::error_handling::Result<void> { + if (nullptr == spec) { + return Struct::ErrorCode{Struct::ErrorCodeEnum::NullStructSpec}; + } + if (nullptr != m_spec) { + return Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (4)
Struct(66-66)name(37-38)indentation_level(41-42)spec(59-60)
tests/tdl/test-parser-ast.cpp (3)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
Struct(66-66)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
create(40-47)create(40-41)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(17-26)create(17-18)
🔇 Additional comments (4)
tests/tdl/test-parser-ast.cpp (2)
25-25: Include of Struct header looks correct and scoped.
Header path aligns with other includes and matches usage below.
41-41: Using-alias for Struct improves readability.
Keeps test code concise and consistent with existing aliases.src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
40-47: Factory validates child type and attaches name correctly. LGTM.
Type validation and child wiring are consistent with the rest of the AST design.
49-61: Serialisation format matches existing conventions.
Indentation and child serialisation align with other node implementations.
| SECTION("Set spec to a wrong Struct") { | ||
| auto struct_result{Struct::create(Identifier::create("WrongStruct"))}; | ||
| REQUIRE_FALSE(struct_result.has_error()); | ||
| auto* struct_node{dynamic_cast<Struct*>(struct_result.value().get())}; | ||
| REQUIRE(nullptr != struct_node); | ||
| auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())}; | ||
| REQUIRE(set_spec_result.has_error()); | ||
| REQUIRE(set_spec_result.error() | ||
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also assert struct state remains unchanged after a failed set_spec due to name mismatch.
Ensures no partial assignment occurs on error.
Apply this diff:
auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())};
REQUIRE(set_spec_result.has_error());
REQUIRE(set_spec_result.error()
== Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch});
+ // Ensure no spec was installed on error.
+ REQUIRE(struct_node->get_spec() == nullptr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SECTION("Set spec to a wrong Struct") { | |
| auto struct_result{Struct::create(Identifier::create("WrongStruct"))}; | |
| REQUIRE_FALSE(struct_result.has_error()); | |
| auto* struct_node{dynamic_cast<Struct*>(struct_result.value().get())}; | |
| REQUIRE(nullptr != struct_node); | |
| auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())}; | |
| REQUIRE(set_spec_result.has_error()); | |
| REQUIRE(set_spec_result.error() | |
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); | |
| } | |
| SECTION("Set spec to a wrong Struct") { | |
| auto struct_result{Struct::create(Identifier::create("WrongStruct"))}; | |
| REQUIRE_FALSE(struct_result.has_error()); | |
| auto* struct_node{dynamic_cast<Struct*>(struct_result.value().get())}; | |
| REQUIRE(nullptr != struct_node); | |
| auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())}; | |
| REQUIRE(set_spec_result.has_error()); | |
| REQUIRE(set_spec_result.error() | |
| == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); | |
| // Ensure no spec was installed on error. | |
| REQUIRE(struct_node->get_spec() == nullptr); | |
| } |
🤖 Prompt for AI Agents
In tests/tdl/test-parser-ast.cpp around lines 442 to 451, after asserting that
set_spec returned a name-mismatch error, add assertions that the Struct object's
state was not mutated: confirm the struct's identifier/name is still
"WrongStruct" and that its spec pointer/member remains unset (or unchanged) so
no partial assignment occurred on error.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/tdl/test-parser-ast.cpp (2)
443-452: Also assert state is unchanged on name-mismatch error.
After a failed set_spec due to StructSpecNameMismatch, ensure no partial assignment occurred and the struct name/spec remain intact.Apply this diff to extend the assertions:
auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())}; REQUIRE(set_spec_result.has_error()); REQUIRE(set_spec_result.error() == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); + // Ensure no spec was installed on error and name is unchanged. + REQUIRE(struct_node->get_spec() == nullptr); + REQUIRE(struct_node->get_name() == "WrongStruct");
389-453: Add a negative test: Struct::create should reject non-Identifier names.
This mirrors the existing container invalid-input tests and exercises validate_child_node_type in Struct::create.Append this SECTION within the "Struct" block (e.g., after the “Set spec to a wrong Struct” section):
SECTION("Set spec to a wrong Struct") { auto struct_result{Struct::create(Identifier::create("WrongStruct"))}; REQUIRE_FALSE(struct_result.has_error()); auto* struct_node{dynamic_cast<Struct*>(struct_result.value().get())}; REQUIRE(nullptr != struct_node); auto const set_spec_result{struct_node->set_spec(struct_spec_result.value())}; REQUIRE(set_spec_result.has_error()); REQUIRE(set_spec_result.error() == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecNameMismatch}); } + + SECTION("Invalid inputs for Struct creation") { + // Non-Identifier as name should be rejected. + auto invalid_name{Int::create(IntSpec::Int64)}; + auto invalid_struct{Struct::create(std::move(invalid_name))}; + REQUIRE(invalid_struct.has_error()); + REQUIRE(invalid_struct.error() + == Node::ErrorCode{Node::ErrorCodeEnum::UnexpectedChildNodeType}); + }
🧹 Nitpick comments (1)
tests/tdl/test-parser-ast.cpp (1)
408-441: Strengthen post-conditions: also assert the installed spec equals the provided shared_ptr.
After a successful set_spec, verify the struct holds the exact same shared_ptr (not just non-null). This tightens the state validation and guards against inadvertent copies of a different instance.Apply this diff inside the same SECTION after setting the spec:
// Set the `StructSpec` to the `Struct` REQUIRE_FALSE(struct_node->set_spec(struct_spec_result.value()).has_error()); REQUIRE(nullptr != struct_node->get_spec()); + // The struct should retain the exact provided spec instance. + REQUIRE(struct_node->get_spec() == struct_spec_result.value()); // Ensure `StructSpec` can't be set again auto const duplicated_set_spec{struct_node->set_spec(struct_spec_result.value())}; REQUIRE(duplicated_set_spec.has_error()); REQUIRE(duplicated_set_spec.error() == Struct::ErrorCode{Struct::ErrorCodeEnum::StructSpecAlreadySet}); + // Spec remains unchanged after duplicate set attempt. + REQUIRE(struct_node->get_spec() == struct_spec_result.value());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/tdl/test-parser-ast.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tdl/test-parser-ast.cpp (3)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
Struct(66-66)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
create(40-47)create(40-41)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(17-26)create(17-18)
⏰ 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (3)
tests/tdl/test-parser-ast.cpp (3)
25-25: Header include for Struct is correct and appropriately scoped.
Matches the new public API location and keeps include grouping consistent with neighbouring type headers.
41-41: Alias for Struct improves readability and consistency.
Keeps usage aligned with other type aliases in this test file.
389-407: Good: Reusable StructSpec fixture and type check before use.
Creating a minimal valid StructSpec once per SECTION path and sanity-checking its dynamic type is clean and avoids duplication.
2 participants
Description
This PR adds
StructAST node as an implementation ofType. AStructhas only one child: the identifier. It also holds a reference to aStructSpecas a shared pointer. However, such a spec is not initialized during the construction. This is because the AST nodes are constructed at the parsing stage. The spec should be checked and set in the semantic analysis stage, while a symbol table is built.Checklist
breaking change.
Validation performed
Struct.Summary by CodeRabbit
New Features
Tests