feat(tdl): Add support for tracking the source location in AST nodes. - #199
Conversation
WalkthroughIntroduces SourceLocation across the AST: adds a new SourceLocation class, threads it into Node and all derived nodes via constructors and create() APIs, updates CMake to expose the header, adjusts base Type/Primitive/Container classes, and revises tests to use the new location-aware factories. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Factories as Node Factories
participant Node as AST Node
participant Base as Base Classes (Node/Type/Primitive/Container)
Parser->>Factories: create(..., SourceLocation)
Factories->>Node: construct Node-derived(..., SourceLocation)
Node->>Base: forward SourceLocation to base ctor
Base-->>Node: store SourceLocation
Node-->>Parser: return node with location
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (
|
There was a problem hiding this comment.
This fix is missing by the CI check.
There was a problem hiding this comment.
Look like the clang-tidy check problems is much broader than just the type_traits problem. However, it provides a clue that the root cause might be if a system header is import through another system header, clang-tidy might not require the former to be included. Need more experiments to test if this is true.
@CodeRabbit, Please refactor #192 to make it broader, and add the hypothesis in the issue.
There was a problem hiding this comment.
🧩 Analysis chain
I'll help you refactor issue #192 to make it broader and add the hypothesis about clang-tidy's behavior with system headers. Let me first find the current issue to understand its scope.
Now I'll update the issue to be broader and include the hypothesis:
@sitaowang1998 I've updated issue #192 to:
- Broadened the scope from just
type_traitsto all system headers with include-what-you-use violations - Added your hypothesis about clang-tidy's behavior with transitive system header imports
- Included the current observations from PR feat(tdl): Add support for tracking the source location in AST nodes. #199 where
<string_view>needed to be added - Outlined a comprehensive investigation plan to test the hypothesis and fix violations systematically
- Added proper backlinks to this PR and comment for traceability
The updated issue now provides a framework for systematically investigating and resolving these clang-tidy violations across the entire codebase, rather than fixing them piecemeal as they're discovered.
🏁 Script executed:
#!/bin/bash
# Find and examine issue #192
gh issue view 192 --json title,body,state,url
Length of output: 1643
🏁 Script executed:
#!/bin/bash
# Update issue #192 with broader scope and hypothesis
gh issue edit 192 --title "Investigate and fix clang-tidy include-what-you-use violations for system headers" --body "## Problem
The CI is failing due to clang-tidy include-what-you-use violations where system headers are being used without being explicitly included. This issue has been observed with multiple system headers, not just \`<type_traits>\`.
## Current Observations
- PR #199 required adding \`#include <string_view>\` to \`src/spider/tdl/parser/ast/utils.hpp\` (Line 6)
- Previous issues have been seen with \`<type_traits>\` and other system headers
## Hypothesis
The root cause might be that when a system header is imported through another system header (transitive inclusion), clang-tidy's include-what-you-use check might not require the former to be explicitly included. However, this behavior may be inconsistent or dependent on specific configurations.
## Required Work
1. **Investigate the scope**: Conduct a comprehensive audit of all files to identify similar violations across the codebase
2. **Test the hypothesis**: Experiment with different system headers to verify if transitive inclusion affects clang-tidy's requirements
3. **Fix violations**: Add explicit includes for all system headers that are being used directly
4. **Document findings**: Update coding guidelines if patterns are discovered
## Related
- PR #199: https://github.com/y-scope/spider/pull/199
- Comment: https://github.com/y-scope/spider/pull/199#discussion_r2283266968
## Assignee
@sitaowang1998"
Length of output: 1553
@sitaowang1998 Perfect! I've successfully refactored issue #192 as requested. Here's what I've accomplished:
✅ Issue Updated Successfully
Original scope: Narrow focus on <type_traits> and std::is_invocable_r_v
New scope: Comprehensive investigation of all clang-tidy include-what-you-use violations for system headers
🔍 Key Changes Made:
-
Broadened the title: "Investigate and fix clang-tidy include-what-you-use violations for system headers"
-
Added your hypothesis: The theory that transitive system header imports may cause inconsistent clang-tidy behavior
-
Included current observations: Referenced the
<string_view>fix needed in this PR (feat(tdl): Add support for tracking the source location in AST nodes. #199) -
Created systematic investigation plan:
- Comprehensive codebase audit
- Hypothesis testing with different system headers
- Systematic violation fixes
- Documentation updates
-
Added proper traceability: Links back to this PR and comment for future reference
The issue is now positioned as a broader investigation that will help establish patterns and potentially prevent future occurrences of these violations across the entire codebase, rather than fixing them reactively as they're discovered.
🧬 Code Graph Analysis Results
-
File: src/spider/tdl/parser/ast/node_impl/Function.hpp
Lines: 50-51
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/NamedVar.hpp
Lines: 35-36
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/Namespace.hpp
Lines: 53-54
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/StructSpec.hpp
Lines: 53-54
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp
Lines: 31-32
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/SourceLocation.hpp
Lines: 13-13
Content:[[nodiscard]] auto get_line() const noexcept -> size_t { return m_line; } -
File: src/spider/tdl/parser/ast/SourceLocation.hpp
Lines: 15-15
Content:[[nodiscard]] auto get_column() const noexcept -> size_t { return m_column; } -
File: src/spider/tdl/parser/ast/node_impl/Identifier.hpp
Lines: 33-33
Content:[[nodiscard]] auto get_name() const noexcept -> std::string_view { return m_name; } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 48-48
Content:[[nodiscard]] auto get_parent() const noexcept -> Node const* { return m_parent; } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 53-53
Content:[[nodiscard]] auto get_num_children() const noexcept -> size_t { return m_children.size(); } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 76-82
Content:[[nodiscard]] auto visit_children(ChildVisitor visitor) const -> ystdlib::error_handling::Result<void> { for (auto const& child : m_children) { YSTDLIB_ERROR_HANDLING_TRYV(visitor(*child)); } return ystdlib::error_handling::success(); } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 91-92
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 95-97
Content:[[nodiscard]] auto get_source_location() const noexcept -> SourceLocation { return m_source_location; } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 119-121
Content:[[nodiscard]] auto get_child_unsafe(size_t child_idx) const -> Node const* { return m_children[child_idx].get(); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp
Lines: 24-27
Content:[[nodiscard]] static auto create(IntSpec spec, SourceLocation source_location) -> std::unique_ptr<Node> { return std::make_unique<Int>(Int{spec, source_location}); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp
Lines: 34-34
Content:[[nodiscard]] auto get_spec() const -> IntSpec { return m_spec; } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp
Lines: 30-31
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp
Lines: 24-25
Content:create(IntSpec spec, SourceLocation source_location) -> std::unique_ptr<Node> -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp
Lines: 24-27
Content:[[nodiscard]] static auto create(FloatSpec spec, SourceLocation source_location) -> std::unique_ptr<Node> { return std::make_unique<Float>(Float{spec, source_location}); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp
Lines: 34-34
Content:[[nodiscard]] auto get_spec() const -> FloatSpec { return m_spec; } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp
Lines: 30-31
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp
Lines: 24-25
Content:create(FloatSpec spec, SourceLocation source_location) -> std::unique_ptr<Node> -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp
Lines: 35-39
Content:[[nodiscard]] auto get_element_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)); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp
Lines: 31-32
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp
Lines: 48-52
Content:[[nodiscard]] auto get_key_type() const -> 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)); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp
Lines: 54-58
Content:[[nodiscard]] auto get_value_type() const -> Type const* { // The factory function ensures that the second child is of type `Type`. // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast) return static_cast<Type const*>(get_child_unsafe(1)); } -
File: src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp
Lines: 44-45
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> override -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 48-48 (duplicate of get_parent)
Content:[[nodiscard]] auto get_parent() const noexcept -> Node const* { return m_parent; } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 53-53 (duplicate of get_num_children)
Content:[[nodiscard]] auto get_num_children() const noexcept -> size_t { return m_children.size(); } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 76-82 (duplicate of visit_children)
Content:[[nodiscard]] auto visit_children(ChildVisitor visitor) const -> ystdlib::error_handling::Result<void> { for (auto const& child : m_children) { YSTDLIB_ERROR_HANDLING_TRYV(visitor(*child)); } return ystdlib::error_handling::success(); } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 91-92
Content:serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 95-97
Content:[[nodiscard]] auto get_source_location() const noexcept -> SourceLocation { return m_source_location; } -
File: src/spider/tdl/parser/ast/Node.hpp
Lines: 119-121
Content:[[nodiscard]] auto get_child_unsafe(size_t child_idx) const -> Node const* { return m_children[child_idx].get(); }
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
10-15: Make SourceLocation trivially constexpr/noexcept-friendlyMinor polish: mark the ctor and accessors constexpr and the ctor noexcept. This enables compile-time construction and can help with constant expressions without changing runtime behaviour.
Apply:
- SourceLocation(size_t line, size_t column) : m_line{line}, m_column{column} {} + constexpr SourceLocation(size_t line, size_t column) noexcept : m_line{line}, m_column{column} {} - [[nodiscard]] auto get_line() const noexcept -> size_t { return m_line; } + [[nodiscard]] constexpr auto get_line() const noexcept -> size_t { return m_line; } - [[nodiscard]] auto get_column() const noexcept -> size_t { return m_column; } + [[nodiscard]] constexpr auto get_column() const noexcept -> size_t { return m_column; }
9-15: Clarify 1-based vs 0-based semantics for line/columnA brief note on whether line/column are 1-based or 0-based will prevent subtle off-by-one inconsistencies across producers/consumers.
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (1)
26-26: Prefer direct-initialization in make_unique to avoid an extra moveIdiomatic and marginally cleaner; constructs the object in-place.
Apply:
- auto named_var{std::make_unique<NamedVar>(NamedVar{source_location})}; + auto named_var{std::make_unique<NamedVar>(source_location)};src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)
10-13: Mark Primitive(SourceLocation) as noexceptConstruction should not throw; marking it noexcept helps optimisation and communicates intent. Derived ctors will inherit noexcept if they don’t throw.
Apply:
- explicit Primitive(SourceLocation source_location) : Type{source_location} {} + explicit Primitive(SourceLocation source_location) noexcept : Type{source_location} {}src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
32-32: Optional: consider perfect-forwarding in make_unique (consistency vs. micro-optimisation)The pattern std::make_unique(Bool{source_location}) relies on move construction. Using direct perfect-forwarding avoids constructing a temporary:
- return std::make_unique<Bool>(Bool{source_location}); + return std::make_unique<Bool>(source_location);If the codebase intentionally standardises on the current pattern (it appears consistent across nodes), feel free to keep it for uniformity.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
84-84: Nit: avoid temporary in make_uniqueMinor allocation/ctor simplification; construct in-place instead of moving a temporary:
- auto map{std::make_unique<Map>(Map{source_location})}; + auto map{std::make_unique<Map>(source_location)};Given other nodes follow the temporary pattern, apply only if you decide to standardise on direct construction.
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
24-26: Mark the factory [[nodiscard]] for consistencyOther node factories are annotated [[nodiscard]]. Do the same here to prevent accidental discard and to keep the API consistent.
Apply this diff:
-static auto create(std::string name, SourceLocation source_location) -> std::unique_ptr<Node> { +[[nodiscard]] static auto create(std::string name, SourceLocation source_location) -> std::unique_ptr<Node> { return std::make_unique<Identifier>(Identifier{std::move(name), source_location}); }src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp (1)
26-28: Minor: consider noexcept on is_empty()Since is_empty delegates to get_num_children() which is noexcept, marking it noexcept clarifies intent and enables minor optimizations.
Outside this hunk, update the method as:
[[nodiscard]] auto is_empty() const noexcept -> bool { return 0 == get_num_children(); }src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
46-50: Fix copy-paste in comment and consider noexcept on get_num_functions()
- The comment in visit_functions currently mentions NamedVar; it should say Function.
- get_num_functions() can be noexcept, similar to other query methods.
Outside this hunk, adjust these spots:
// In visit_functions loop above: // The factory function ensures that all the child nodes are `Function` except the first one. [[nodiscard]] auto get_num_functions() const noexcept -> size_t { return get_num_children() - 1; }src/spider/tdl/parser/ast/node_impl/Function.cpp (1)
66-66: Minor: avoid temporary object in make_uniqueConstructing via
std::make_unique<Function>(Function{...})creates a temporary and relies on move/copy construction. Prefer direct forwarding of constructor args.Apply this diff:
- auto function{std::make_unique<Function>(Function{has_return, source_location})}; + auto function = std::make_unique<Function>(has_return, source_location);src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (1)
67-67: Minor: avoid temporary object in make_sharedSimilar to other spots, constructing via
std::make_shared<StructSpec>(StructSpec{...})creates a temporary. Prefer direct forwarding.Apply this diff:
- auto struct_spec{std::make_shared<StructSpec>(StructSpec{source_location})}; + auto struct_spec = std::make_shared<StructSpec>(source_location);Additionally, consider reserving capacity in the set to avoid rehashes for larger structs (optional):
// Near the declaration of field_names absl::flat_hash_set<std::string_view> field_names; field_names.reserve(fields.size());src/spider/tdl/parser/ast/node_impl/Namespace.cpp (1)
66-72: Rename local variablefunctionto avoid confusion.
The variable holds a Namespace node; naming itfunctionis misleading next tofunc. Minor readability nit.Apply:
- auto function{std::make_unique<Namespace>(Namespace{source_location})}; - YSTDLIB_ERROR_HANDLING_TRYV(function->add_child(std::move(name))); + auto ns{std::make_unique<Namespace>(Namespace{source_location})}; + YSTDLIB_ERROR_HANDLING_TRYV(ns->add_child(std::move(name))); @@ - YSTDLIB_ERROR_HANDLING_TRYV(function->add_child(std::move(func))); + YSTDLIB_ERROR_HANDLING_TRYV(ns->add_child(std::move(func))); } - return function; + return ns;src/spider/tdl/parser/ast/Node.hpp (1)
95-98: Optional: consider returning a const reference from get_source_location().
SourceLocationlooks small; returning by value is fine. If it ever grows, a const ref avoids copies. Your call.- [[nodiscard]] auto get_source_location() const noexcept -> SourceLocation { - return m_source_location; - } + [[nodiscard]] auto get_source_location() const noexcept -> SourceLocation const& { + return m_source_location; + }tests/tdl/test-parser-ast.cpp (5)
55-59: Use a single test SourceLocation and consider a non-zero valueRight now, every factory call reconstructs a zero location. Consider:
- Using a single constexpr/static SourceLocation to reduce repetition/noise.
- Using a non-zero location to better detect plumbing mistakes that default to zero.
Example tweak (optional):
-[[nodiscard]] auto create_source_location() -> spider::tdl::parser::ast::SourceLocation; +[[nodiscard]] inline auto create_source_location() -> spider::tdl::parser::ast::SourceLocation; @@ -auto create_source_location() -> spider::tdl::parser::ast::SourceLocation { - return spider::tdl::parser::ast::SourceLocation{0, 0}; -} +auto create_source_location() -> spider::tdl::parser::ast::SourceLocation { + return spider::tdl::parser::ast::SourceLocation{42, 7}; +}Also applies to: 104-107
130-140: Add an assertion for Identifier’s SourceLocationSince this PR’s goal is location plumbing, add a minimal assertion to verify it for Identifier.
Apply this diff:
REQUIRE(nullptr == identifier->get_parent()); REQUIRE(identifier->get_name() == cTestName); + + // Verify SourceLocation is threaded + auto const loc = identifier->get_source_location(); + REQUIRE(loc.get_line() == 0); + REQUIRE(loc.get_column() == 0);If you adopt a non-zero location in create_source_location(), adjust expected values accordingly.
174-184: Add an assertion for Float’s SourceLocationVerify the Float node carries the provided SourceLocation.
REQUIRE(float_node->get_spec() == float_spec); REQUIRE(float_node->get_num_children() == 0); + + // Verify SourceLocation is threaded + auto const loc = float_node->get_source_location(); + REQUIRE(loc.get_line() == 0); + REQUIRE(loc.get_column() == 0);
200-207: Add a location check for List (container) nodesA quick assertion here increases confidence that containers also carry locations properly.
auto const* list_node{dynamic_cast<List const*>(list_result.value().get())}; REQUIRE(nullptr != list_node); + // Verify SourceLocation is threaded + auto const list_loc = list_node->get_source_location(); + REQUIRE(list_loc.get_line() == 0); + REQUIRE(list_loc.get_column() == 0); + REQUIRE(list_node->get_num_children() == 1);Also applies to: 208-225
239-247: Add a location check for Map nodesSimilarly, assert SourceLocation on Map.
auto const* map_node{dynamic_cast<Map const*>(map_result.value().get())}; REQUIRE(nullptr != map_node); + // Verify SourceLocation is threaded + auto const map_loc = map_node->get_source_location(); + REQUIRE(map_loc.get_line() == 0); + REQUIRE(map_loc.get_column() == 0); + REQUIRE(map_node->get_num_children() == 2);
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
src/spider/CMakeLists.txt(1 hunks)src/spider/tdl/parser/ast/Node.hpp(3 hunks)src/spider/tdl/parser/ast/SourceLocation.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Function.cpp(3 hunks)src/spider/tdl/parser/ast/node_impl/Function.hpp(4 hunks)src/spider/tdl/parser/ast/node_impl/Identifier.hpp(2 hunks)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp(3 hunks)src/spider/tdl/parser/ast/node_impl/Namespace.cpp(3 hunks)src/spider/tdl/parser/ast/node_impl/Namespace.hpp(3 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp(3 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp(3 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/Struct.cpp(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp(3 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(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp(3 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp(3 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp(2 hunks)src/spider/tdl/parser/ast/utils.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(27 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T14:54:20.267Z
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.267Z
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.hpp
🧬 Code Graph Analysis (24)
src/spider/tdl/parser/ast/node_impl/Type.hpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/SourceLocation.hpp (1)
src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (3)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
create(17-24)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (1)
key_type(37-41)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (10)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(18-30)create(18-22)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
create(17-24)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
create(20-31)create(20-21)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (2)
name(24-26)name(24-24)src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
name(42-47)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
name(39-40)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)src/spider/tdl/parser/ast/utils.hpp (3)
validate_child_node_type(53-54)validate_child_node_type(58-63)validate_child_node_type(58-58)
src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/Namespace.hpp (8)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (3)
nodiscard(33-33)name(24-26)name(24-24)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (2)
nodiscard(39-43)nodiscard(45-49)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/Namespace.cpp (2)
name(27-29)name(27-27)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
name(39-40)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (4)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (3)
nodiscard(33-33)name(24-26)name(24-24)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (8)
src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
name(42-47)src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
name(27-29)name(27-27)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/Namespace.cpp (2)
name(27-29)name(27-27)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
name(39-40)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
name(24-26)name(24-24)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (6)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
nodiscard(22-24)source_location(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (2)
nodiscard(48-52)nodiscard(54-58)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (2)
nodiscard(24-27)nodiscard(34-34)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (2)
nodiscard(24-27)nodiscard(34-34)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (4)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
nodiscard(22-24)source_location(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
nodiscard(24-27)nodiscard(34-34)spec(24-25)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (7)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(18-30)create(18-22)src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
create(42-75)create(42-47)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
create(45-73)create(45-49)src/spider/tdl/parser/ast/node_impl/Namespace.cpp (2)
create(44-72)create(44-48)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
create(17-24)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
create(70-88)create(70-74)src/spider/tdl/parser/ast/utils.hpp (3)
validate_child_node_type(53-54)validate_child_node_type(58-63)validate_child_node_type(58-58)
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (4)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
name(39-40)
src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (4)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (2)
nodiscard(24-27)nodiscard(34-34)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (2)
nodiscard(24-27)nodiscard(34-34)
src/spider/tdl/parser/ast/node_impl/Function.cpp (1)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (5)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
create(70-88)create(70-74)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
create(20-31)create(20-21)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1)
element_type(27-28)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)src/spider/tdl/parser/ast/utils.hpp (3)
validate_child_node_type(53-54)validate_child_node_type(58-63)validate_child_node_type(58-58)
src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (5)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
nodiscard(33-33)src/spider/tdl/parser/ast/node_impl/Function.hpp (6)
nodiscard(54-54)nodiscard(56-60)nodiscard(65-72)nodiscard(74-76)nodiscard(90-103)nodiscard(112-114)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (2)
nodiscard(57-61)nodiscard(63-63)
src/spider/tdl/parser/ast/Node.hpp (3)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (2)
nodiscard(24-27)nodiscard(34-34)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (2)
nodiscard(24-27)nodiscard(34-34)
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (11)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(18-30)create(18-22)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
create(17-24)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (4)
create(41-48)create(41-42)name(22-24)name(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (4)
create(70-88)create(70-74)name(24-26)name(24-24)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
create(20-31)create(20-21)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (2)
name(24-26)name(24-24)src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
name(42-47)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
name(39-40)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
nodiscard(22-24)source_location(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (3)
nodiscard(24-27)nodiscard(34-34)spec(24-25)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (4)
src/spider/tdl/parser/ast/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (2)
nodiscard(22-24)source_location(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (2)
nodiscard(24-27)nodiscard(34-34)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (2)
nodiscard(24-27)nodiscard(34-34)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (8)
src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
create(42-75)create(42-47)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
create(45-73)create(45-49)src/spider/tdl/parser/ast/node_impl/Namespace.cpp (2)
create(44-72)create(44-48)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (2)
create(17-24)create(17-18)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
create(70-88)create(70-74)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
create(20-31)create(20-21)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (1)
id(31-32)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
source_location(22-22)
src/spider/tdl/parser/ast/node_impl/Namespace.cpp (5)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (2)
create(18-30)create(18-22)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (4)
create(45-73)create(45-49)name(27-29)name(27-27)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (2)
name(24-26)name(24-24)src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
name(42-47)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
name(46-50)
⏰ 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (36)
src/spider/tdl/parser/ast/utils.hpp (1)
6-6: serialize_ functions safely return string literals*
Verified that bothserialize_int_specandserialize_float_speconly return string literals (e.g.,"int8","float"), which have static storage duration. Thestd::string_views therefore refer to stable memory and won’t dangle. No further changes needed.src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (1)
18-22: AllNamedVar::createcall sites updated withSourceLocationVerified that every occurrence of
NamedVar::createnow passes aSourceLocationargument:
- tests/tdl/test-parser-ast.cpp:78, 329, 413, 431, 489, 526
No further changes are required.
src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)
5-5: Include addition is appropriateIncluding SourceLocation.hpp here matches the new constructor signature and the Type(SourceLocation) base.
src/spider/tdl/parser/ast/node_impl/Type.hpp (1)
9-13: LGTM: Type now correctly threads SourceLocation via a protected ctorExplicit, protected ctor forwarding SourceLocation to Node is the right shape for an abstract base. Keeps construction disciplined in derived classes.
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (1)
41-47: LGTM: Factory threads SourceLocation and preserves existing invariantscreate now accepts SourceLocation and constructs Struct{source_location}, while retaining child-type validation and attach order. Matches the pattern used in other nodes in this PR.
src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp (1)
9-13: LGTM: Container properly forwards SourceLocation to TypeProtected explicit ctor enforces intended inheritance-only construction and cleanly propagates location metadata to the base.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
22-24: LGTM: Location-aware Bool factoryFactory signature and construction align with the new SourceLocation plumbing across the AST.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
70-75: LGTM: Map::create now location-aware with consistent validationSignature and ctor usage correctly include SourceLocation; child-type validation and key-type gate remain unchanged. Good consistency with List/Tuple.
src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (1)
13-13: Location threading and constructor change look correctHeader inclusion, factory signature, and explicit constructor forwarding to Node{source_location} are consistent with the rest of the PR and the updated Node API.
Also applies to: 25-25, 30-32, 53-53
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp (1)
13-13: Factory signature and constructor update align with the new location-aware NodeThe added SourceLocation include, factory parameter, and explicit constructor forwarding to Container{source_location} look good and match the broader changes.
Also applies to: 21-21, 26-28, 39-39
src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
18-18: Location support integration looks solidHeader inclusion, factory signature addition, and explicit constructor forwarding to Node{source_location} are correct and consistent with other nodes.
Also applies to: 36-39, 46-50, 92-92
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (1)
16-16: Tuple::create correctly threads SourceLocationValidation remains unchanged, and construction now passes the SourceLocation to Tuple; consistent with List/Map/Struct. Looks good.
Also applies to: 20-21, 26-26
src/spider/tdl/parser/ast/node_impl/Function.cpp (1)
20-20: Include for SourceLocation is correct and necessaryThe added include is appropriate for the new parameter and constructor usage.
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (3)
18-18: Include for SourceLocation is correctly addedHeader now declares the new dependency; good.
46-51: API change: StructSpec::create now takes SourceLocationThe new factory signature is clear and consistent with the rest of the AST APIs (location as the last parameter).
92-92: Base Node initialisation with SourceLocation is correctThe private constructor correctly forwards the SourceLocation to Node.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (3)
13-13: Include for SourceLocation is appropriateRequired for the new constructor/factory signatures.
24-27: Factory now threads SourceLocation (good consistency with Bool/Float)The create() overload and return type remain consistent with other primitive nodes.
38-38: Constructor threads SourceLocation via Primitive base correctlyInitializes the base with SourceLocation and stores the spec; looks good.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (1)
13-13: List now carries SourceLocation through factory and construction
- Include added.
- Factory signature updated to take SourceLocation.
- Instance constructed with the provided location.
All changes align with the broader AST changes.
Also applies to: 17-17, 21-21
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (2)
19-19: Include for SourceLocation is correctNeeded for the new factory parameter and constructor.
45-49: All StructSpec::create call sites now include the new SourceLocation parameterWe searched the entire codebase for
StructSpec::createand found only the definition plus four test usages—all of which passcreate_source_location()as the third argument:• tests/tdl/test-parser-ast.cpp:443–446
• tests/tdl/test-parser-ast.cpp:500–503
• tests/tdl/test-parser-ast.cpp:511–514
• tests/tdl/test-parser-ast.cpp:535–538No other call sites were found, so no further updates are needed.
src/spider/tdl/parser/ast/node_impl/Namespace.cpp (1)
20-20: Good addition: SourceLocation include aligns with new location-aware API.
This keeps this TU consistent with the rest of the AST changes.src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
19-19: Please confirm allFunction::createcalls include the newSourceLocationparameter
I ran a search across C++ sources and headers and found theFunction::createdefinition in
- src/spider/tdl/parser/ast/node_impl/Function.cpp
and multiple call sites in
- tests/tdl/test-parser-ast.cpp (e.g. around lines 94, 621, 674, 721, 763, 793)
Some invocations already pass
create_source_location()at the end, but earlier uses (around line 94) didn’t show it in the brief snippet. Please manually verify that everyFunction::create(…)invocation now passes aSourceLocationas its final argument.Suggested command:
rg -n -C2 -g '*.{cpp,cc,cxx,h,hh,hpp,hxx}' '\bFunction::create\s*\('src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
17-17: Struct becomes location-aware — consistent and idiomatic.
- Includes SourceLocation.
- Factory signature and docs updated.
- Ctor forwards to Type with SourceLocation.
Also applies to: 34-35, 39-39, 68-68
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1)
13-13: List: SourceLocation propagation is consistent with other container nodes.
- Header include, factory, and ctor updates all look correct.
Also applies to: 21-22, 26-28, 43-43
src/spider/tdl/parser/ast/Node.hpp (2)
14-15: Base Node now includes SourceLocation — correct placement and visibility.
This brings the type into scope for the base class API and derived classes.
101-101: Verify that all factory create() methods accept a SourceLocationNo initializer‐lists still default‐constructing
Node{}and no explicit calls toNode()were found. The automated search forcreate…()functions proved noisy, so please manually confirm that every factory method insrc/spider/tdl/parser/ast/node_impl(for example):
- StructSpec::create (
.../node_impl/StructSpec.cpp)- NamedVar::create (
.../node_impl/NamedVar.cpp)- Namespace::create (
.../node_impl/Namespace.cpp)- Function::create (
.../node_impl/Function.cpp)- Map::create (
.../node_impl/type_impl/container_impl/Map.cpp)includes a
SourceLocationparameter in its signature.src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (3)
15-15: Include SourceLocation – LGTMIncluding SourceLocation in the public header is appropriate since the type appears in the public API.
62-62: Constructor correctly forwards SourceLocation – LGTMThe explicit constructor forwarding SourceLocation into Container aligns with the new Node plumbing.
31-41: API change verified: no non-test callers of Map::createAll
Map::createinvocations found are in tests and have been updated to pass the newSourceLocationparameter. No call sites in production code were detected.• tests/tdl/test-parser-ast.cpp – all
Map::create(…, create_source_location())calls updated
• src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp – factory signature includesSourceLocationPlease remember to document this public API change in the release notes.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (3)
13-13: Include SourceLocation – LGTMIncluding SourceLocation here is necessary and consistent with the other primitive nodes.
24-27: Factory updated to accept SourceLocation – LGTMSignature mirrors Int::create and Bool::create; construction path is correct.
38-40: Constructor wires SourceLocation into Primitive – LGTMBase initialization order and member initialization are correct.
tests/tdl/test-parser-ast.cpp (2)
28-28: Test dependency update – LGTMIncluding SourceLocation in tests is expected given the location-aware APIs.
295-305: Solid coverage of Map key-type errors – LGTMThese checks for UnsupportedKeyType vs UnexpectedChildNodeType cover the intended error propagation.
There was a problem hiding this comment.
Look like the clang-tidy check problems is much broader than just the type_traits problem. However, it provides a clue that the root cause might be if a system header is import through another system header, clang-tidy might not require the former to be included. Need more experiments to test if this is true.
@CodeRabbit, Please refactor #192 to make it broader, and add the hypothesis in the issue.
2 participants
Description
This PR adds support for tracking the source location in AST nodes by storing a
SourceLocationinstance in each AST node. All node implementations are updated to accept a source location through the factory function.This PR doesn't update the serialization methods to include the source information. We will update this in a future PR to avoid making this PR too long.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Tests