Fixes for IDL maps#2378
Conversation
- Support for parsing bounded maps - Support for using maps in more places - Fix memory leaks caused by maps
WalkthroughThis update adds partial support for bounded maps in TAO_IDL, enabling parsing of bounded maps and their use in unions, sequences, and arrays. It fixes memory leaks related to maps and extends AST and visitor classes to handle map node types. Test IDL and code were updated to exercise bounded maps and map-containing types. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser
participant AST
participant Visitor
User->>Parser: Define bounded map (e.g., map<string, int32, 4>)
Parser->>AST: Create AST_Map node with bound
AST->>Visitor: Accept visitor for map node
Visitor->>Visitor: Call visit_map()
Visitor->>AST: Process map node (ownership, wstring checks, code generation)
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
🔭 Outside diff range comments (1)
TAO/TAO_IDL/fe/idl.ypp (1)
4015-4024: 🛠️ Refactor suggestionSame leak in un-bounded alternative
The un-bounded branch replicates the leak discussed above – please destroy
snhere as well.map = idl_global->gen ()->create_map (...); map->key_type_annotations (*key_type->annotations); map->value_type_annotations (*val_type->annotations); idl_global->err ()->anonymous_type_diagnostic (); + + sn.destroy ();
🧹 Nitpick comments (2)
TAO/TAO_IDL/fe/idl.ypp (2)
3924-3933: Pop condition is brittle
idl_global->scopes ().top () == 0assumes the marker you pushed is still on the top.
If future grammar changes ever push another (non-null) scope before completing the map rule, this test becomes false and the marker will never be removed, breaking later look-ups.Consider unconditionally popping, or – better – wrap the marker push/pop in a small RAII helper so lifetime is guaranteed.
3949-3980: Duplicated maple-creation logic – hard to maintainThe bounded and un-bounded branches differ only in how
max_sizeis built, yet they replicate ~40 lines of near-identical code (type checks, diagnostics, annotation copies, clean-up).Factor the common part into a
create_map(AST_Type*, AST_Type*, AST_Expression*)helper to reduce risk of future divergences and make both code paths safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
TAO/NEWS(1 hunks)TAO/TAO_IDL/ast/ast_array.cpp(1 hunks)TAO/TAO_IDL/ast/ast_decl.cpp(2 hunks)TAO/TAO_IDL/ast/ast_field.cpp(2 hunks)TAO/TAO_IDL/ast/ast_sequence.cpp(2 hunks)TAO/TAO_IDL/be/be_visitor_array/array.h(1 hunks)TAO/TAO_IDL/be/be_visitor_array/cdr_op_cs.cpp(1 hunks)TAO/TAO_IDL/be/be_visitor_sequence/buffer_type.cpp(1 hunks)TAO/TAO_IDL/be/be_visitor_sequence/sequence.h(1 hunks)TAO/TAO_IDL/be_include/be_visitor_array/cdr_op_cs.h(1 hunks)TAO/TAO_IDL/be_include/be_visitor_sequence/buffer_type.h(1 hunks)TAO/TAO_IDL/fe/idl.ypp(2 hunks)TAO/tests/IDLv4/maps/main.cpp(1 hunks)TAO/tests/IDLv4/maps/test.idl(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
TAO/TAO_IDL/be/be_visitor_sequence/buffer_type.cpp (1)
TAO/TAO_IDL/be/be_visitor_array/cdr_op_cs.cpp (2)
visit_map(520-524)visit_map(521-521)
TAO/TAO_IDL/be/be_visitor_array/cdr_op_cs.cpp (2)
TAO/TAO_IDL/be/be_visitor_sequence/buffer_type.cpp (2)
visit_map(89-93)visit_map(90-90)TAO/TAO_IDL/be_include/be_visitor_array/cdr_op_cs.h (14)
node(39-39)node(42-42)node(43-43)node(44-44)node(45-45)node(46-46)node(47-47)node(48-48)node(49-49)node(50-50)node(51-51)node(52-52)node(53-53)node(54-54)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2022Release64
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: MINGW64-gcc
- GitHub Check: windows-2019 vs2019
- GitHub Check: ubuntu-22.04 g++-12
- GitHub Check: macos-13-C++
- GitHub Check: macos-14-C++
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: alpine-3.18
🔇 Additional comments (25)
TAO/TAO_IDL/be/be_visitor_sequence/sequence.h (1)
26-26: Clean addition of map support to sequence visitor.Adding the include for "be_map.h" enables map-related declarations in the sequence visitor, which is necessary for supporting maps in sequences as mentioned in the release notes.
TAO/TAO_IDL/be/be_visitor_array/array.h (1)
26-26: Good integration of map support in array visitor.The include for "be_map.h" is properly added to the array visitor header, maintaining alphabetical order in the include list and enabling map support in array contexts.
TAO/NEWS (1)
4-8: Clear and concise documentation of map enhancements.The release notes appropriately document the three key improvements to IDL maps:
- Support for parsing bounded maps
- Expanded usage of maps in more contexts
- Memory leak fixes related to maps
This provides good context for the code changes seen in the other files.
TAO/TAO_IDL/be_include/be_visitor_sequence/buffer_type.h (1)
56-56: Proper extension of visitor pattern for map support.The addition of the virtual
visit_mapmethod follows the established visitor pattern in the class, extending the visitor's capability to handle map nodes when processing sequence buffer types.TAO/TAO_IDL/be/be_visitor_sequence/buffer_type.cpp (1)
89-93: Map support added to sequence buffer type visitorThis addition properly extends the visitor pattern to handle map nodes, following the same pattern used for other IDL types like sequences. The implementation delegates to the shared
visit_nodemethod, which is consistent with how other node types are handled in this class.TAO/TAO_IDL/be/be_visitor_array/cdr_op_cs.cpp (1)
520-524: Map support added to array CDR operator visitorThis implementation correctly extends the visitor to handle map nodes by delegating to the existing
visit_nodemethod, consistent with the pattern used for other node types in this visitor class. This change is necessary for proper code generation for arrays containing map elements.TAO/tests/IDLv4/maps/main.cpp (1)
25-25: Memory management improvement for sequence testThe addition of the
releaseparameter (set totrue) explicitly transfers ownership of the allocated buffer to the sequence, improving memory management. This change is particularly important in the context of the PR's focus on fixing memory leaks related to maps.TAO/TAO_IDL/ast/ast_array.cpp (1)
228-233: Added proper ownership handling for map types in arraysThis change ensures that if an array's base type is a map, the array takes ownership of it (setting
owns_base_type_ = true). This is essential for proper memory management, as it ensures the array will correctly destroy and delete the map base type in its destructor, preventing memory leaks.TAO/TAO_IDL/be_include/be_visitor_array/cdr_op_cs.h (1)
50-50: Proper extension of visitor for map supportThe addition of the
visit_mapvirtual method correctly extends the CDR operator code generator to handle arrays containing map types. This change is aligned with the PR objective of enabling maps in more contexts within the codebase.TAO/TAO_IDL/ast/ast_field.cpp (2)
106-106: Memory management extended to map typesThis change appropriately extends the ownership semantics to include map types, ensuring proper memory management when maps are used as field types. This directly addresses the PR objective of fixing memory leaks caused by maps.
144-144: Consistent ownership handling in subclass constructorExtending the ownership flag to include map types in the subclass constructor ensures consistent memory management behavior across all AST_Field implementations. This parallels the change in the primary constructor and prevents memory leaks.
TAO/TAO_IDL/ast/ast_sequence.cpp (2)
144-144: Map ownership support for sequencesThis change properly extends the sequence's ownership semantics to include map types, ensuring that sequences containing maps will correctly manage their memory. This helps address the memory leak issues mentioned in the PR objectives.
184-185: Recursion detection enhanced for mapsAdding map types to the recursion detection logic correctly extends the compiler's ability to handle recursive type definitions involving maps. This is necessary for supporting maps in complex type scenarios and prevents potential infinite recursion issues during code generation.
TAO/TAO_IDL/ast/ast_decl.cpp (2)
71-71: Required include for map supportAdding the include for the map header file is necessary for the subsequent map-related code changes in this file.
1498-1504: Proper wstring detection in mapsThis implementation correctly extends the
contains_wstring()method to check both key and value types of maps for wstring usage. This ensures that maps containing wide strings are properly handled during code generation, which is essential for correct marshaling and unmarshaling of data.TAO/tests/IDLv4/maps/test.idl (10)
4-4: Testing recursive map structures looks good.The
iMapRfield demonstrates map usage in recursive structures, which is an important test case to ensure the IDL compiler handles circular references correctly.
8-9: Basic structure fields are properly defined.Simple primitive fields in
TestStructare well structured to be used as values in various map types throughout the test file.
17-17: Important implementation note about bounded maps.This comment provides valuable clarification that even though the IDL syntax supports bounded maps, the underlying C++
std::mapimplementation doesn't enforce these bounds.
19-20: Good test coverage for basic map types.The inclusion of both unbounded and bounded maps with simple key-value types provides good test coverage for the basic map functionality.
22-23: Comprehensive testing of maps with struct values.These entries demonstrate maps with structure values, with both unbounded and bounded variants, which is essential for testing complex value types in maps.
25-26: Testing maps with sequence values is thorough.Including maps with sequence values tests another level of complexity, ensuring the IDL compiler can handle nested container types correctly.
28-29: Good coverage of nested maps.Testing maps of maps (nested maps) is critical to ensure proper handling of complex nested container types by the IDL compiler.
31-31: Innovative map key type test.Using another map type (
TestIntStringMap) as a key is a good edge case to test and ensures the IDL compiler can handle complex key types.
33-34: Expanded map usage in containers.Adding maps in arrays and sequences addresses the PR objective of enabling maps in more contexts. This is essential for testing how maps interact with other container types.
37-41: Union with map members is well constructed.The
UnionWithMapdefinition properly demonstrates both named and anonymous map usage within a union, which is a good test case for the IDL compiler's handling of maps in discriminated unions.
| map<uint32, TestStructMap, 2> uint32MapMapBounded; | ||
|
|
||
| map<TestIntStringMap, string> mapStringMap; | ||
| map<TestIntStringMap, string> mapStringMap; |
There was a problem hiding this comment.
Does a map with a map key work?
There was a problem hiding this comment.
Yes, apparently maps have comparison operators.
There was a problem hiding this comment.
Although IDL43-70 isn't finalized yet, I don't think they'll be supported in IDL.
Not a problem with it being tested here -- but we may not end up allowing them in OpenDDS.
There was a problem hiding this comment.
This isn't a IDL issue though, it's a mapping issue and for C++ at least it would be solved by providing comparison operators.
|
@jwillemsen do you know why this Codacy check can't find a header? https://app.codacy.com/gh/DOCGroup/ACE_TAO/pull-requests/2378/issues#issue-f51c39f1d24759cc3eca0836a628c14c |
Yeah, yeah it is. 🫤 |
No idea why |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp (1)
114-139:⚠️ Potential issueMissing conditional handling for map types.
The
visit_seq_map_commonimplementation only creates abe_visitor_sequence_csvisitor regardless of whether the node is a sequence or a map. This is inconsistent with how other visitors (like in public_ch.cpp) handle the different node types.Consider implementing proper type checking and visitor selection:
- be_visitor_sequence_cs visitor (&ctx); - - if (node->accept (&visitor) == -1) - { - ACE_ERROR_RETURN ((LM_ERROR, - "(%N:%l) be_visitor_union_branch_public_cs::" - "visit_seq_map_common - " - "codegen failed\n"), - -1); - } + switch (node->node_type ()) + { + case AST_Decl::NT_sequence: + { + be_visitor_sequence_cs visitor (&ctx); + if (node->accept (&visitor) == -1) + { + ACE_ERROR_RETURN ((LM_ERROR, + "(%N:%l) be_visitor_union_branch_public_cs::" + "visit_seq_map_common - " + "sequence codegen failed\n"), + -1); + } + } + break; + case AST_Decl::NT_map: + { + be_visitor_map_cs visitor (&ctx); + if (node->accept (&visitor) == -1) + { + ACE_ERROR_RETURN ((LM_ERROR, + "(%N:%l) be_visitor_union_branch_public_cs::" + "visit_seq_map_common - " + "map codegen failed\n"), + -1); + } + } + break; + default: + { + ACE_ERROR_RETURN ((LM_ERROR, + "(%N:%l) be_visitor_union_branch_public_cs::" + "visit_seq_map_common - " + "invalid type: %C\n", node->node_type_name ()), + -1); + } + };TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp (1)
495-573: 💡 Verification agent🧩 Analysis chain
Potential compile error in error-path & minor robustness issues
node->node_type_name ()is not a member ofbe_type/AST_Decl– the static helper should be used instead.kindis only initialised inside theis_childbranch; if that branch is skipped (typedef or external map/seq) the variable remainsnullptr, yet later dereferenced when building the setter typedef.be_visitor_map_chis used but the corresponding header is not included in this translation unit; some compilers require it.@@ - { - ACE_ERROR_RETURN ((LM_ERROR, - "(%N:%l) be_visitor_union_branch_public_ch::" - "visit_seq_map_common - " - "invalid type: %C\n", node->node_type_name ()), - -1); - } + { + ACE_ERROR_RETURN ((LM_ERROR, + "(%N:%l) be_visitor_union_branch_public_ch::" + "visit_seq_map_common - " + "invalid type: %C\n", + AST_Decl::node_type_name (node->node_type ())), + -1); + } @@ - *os << be_nl_2 - << "typedef " << bt->nested_type_name (bu) - << " _" << ub->local_name () << "_" << kind << ";"; + // `kind` is guaranteed to be set for sequence / map defined + // inside the union. Defensive check avoids UB in case a future + // path forgets to assign it. + if (kind != nullptr) + { + *os << be_nl_2 + << "typedef " << bt->nested_type_name (bu) + << " _" << ub->local_name () << "_" << kind << ";"; + }Follow-ups
• Add#include "be_visitor_map_ch.h"(or the appropriate header) at the top of this file.
• Verify thatpublic_ch.hdeclaresvisit_map.
🏁 Script executed:
#!/bin/bash # Locate the visit_seq_map_common method and show surrounding lines grep -R "visit_seq_map_common" -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp -A20 -B10 # Check where 'kind' is declared and used grep -R "const char\\* kind" -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp -A5 -B5 # Verify inclusion of the map visitor header grep -R "#include" -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp | grep "map_ch" # Search for node_type_name usage and definition grep -R "node_type_name" -n TAO/TAO_IDLLength of output: 4480
🏁 Script executed:
#!/bin/bash # Check includes in public_ch.cpp grep -R '^\s*#\s*include' -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp # Search for the map visitor header in the repo (to know the path) grep -R 'be_visitor_map_ch.h' -n TAO/TAO_IDL # Inspect the class declaration and visit_map in public_ch.h grep -R 'class be_visitor_union_branch_public_ch' -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.h -A5 -B5 grep -R 'visit_map\s*(' -n TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.hLength of output: 519
🏁 Script executed:
#!/bin/bash # List files in the union_branch directory find TAO/TAO_IDL/be/be_visitor_union_branch -maxdepth 1 -type f # Locate sequence and map visitor header files in the repo grep -R "be_visitor_sequence_ch.h" -n TAO/TAO_IDL grep -R "be_visitor_map_ch.h" -n TAO/TAO_IDL # Check includes in union_branch.h for sequence_ch or map_ch headers grep -R "#include .*sequence_ch" -n TAO/TAO_IDL/be/be_visitor_union_branch/union_branch.h grep -R "#include .*map_ch" -n TAO/TAO_IDL/be/be_visitor_union_branch/union_branch.hLength of output: 896
🏁 Script executed:
#!/bin/bash # Display the beginning of union_branch.h to inspect includes and class declarations sed -n '1,200p' TAO/TAO_IDL/be/be_visitor_union_branch/union_branch.hLength of output: 1178
Include missing visitor headers in public_ch.cpp
The implementation in
public_ch.cppcalls bothbe_visitor_sequence_chandbe_visitor_map_chbut only pulls inunion_branch.h(which brings in the non-_chvisitor headers). This will lead to compile errors on some toolchains. No other changes (thenode_type_name()call andkindlogic) need adjustment.Please update
TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cppto add the following at the top, e.g. immediately after includingunion_branch.h:#include "union_branch.h" +#include "be_visitor_sequence_ch.h" +#include "be_visitor_map_ch.h"
♻️ Duplicate comments (1)
TAO/TAO_IDL/fe/idl.ypp (1)
3913-4041: 🛠️ Refactor suggestion
⚠️ Potential issue
UTL_ScopedNameleak persists & template-parameter type not validated
Memory leak (re-issue)
The stack-allocatedsnowns heap memory via its internalIdList.
It is never released in either the bounded or unbounded branch, so each
parsedmapleaks a few bytes – exactly the problem raised in an earlier
review. Callsn.destroy()once the node has been passed to the AST.Missing template-parameter check
Whenmax_sizeis a template parameter (param_holder != 0) the code
should ensure the parameter’s declaredconst_type_isEV_ulong
(see the sequence implementation a few lines below for the correct logic).
Without this, IDL that violates §7.7.4.4 can silently compile and later
break the back-end.Suggested patch (apply to both branches):
@@ - map->value_type_annotations (*val_type->annotations); - - idl_global->err ()->anonymous_type_diagnostic (); + map->value_type_annotations (*val_type->annotations); + + idl_global->err ()->anonymous_type_diagnostic (); + + // Prevent memory-leak of Identifier/IdList created above + sn.destroy (); @@ - AST_Expression::AST_ExprValue *ev = nullptr; + AST_Expression::AST_ExprValue *ev = nullptr; AST_Param_Holder *param_holder = nullptr; @@ - ev = max_size->coerce (AST_Expression::EV_ulong); + ev = max_size->coerce (AST_Expression::EV_ulong); + + // Ensure template parameter used for the bound is of type unsigned long + if (param_holder && + param_holder->info ()->const_type_ != AST_Expression::EV_ulong) + { + idl_global->err ()->mismatched_template_param ( + param_holder->info ()->name_.c_str ()); + }(The same
sn.destroy()and parameter-type guard should be added to the
'>'alternative immediately below.)
🧹 Nitpick comments (3)
TAO/TAO_IDL/be/be_visitor_union_branch/public_reset_cs.cpp (1)
400-426: Parameter inconsistency in common helper method.Unlike the other visitor implementations where
visit_seq_map_commontakes a node parameter, this implementation doesn't. While it works here because the implementation doesn't use the node parameter, it makes the interface inconsistent with other similar visitors.Consider adding the node parameter for consistency:
-int -be_visitor_union_branch_public_reset_cs::visit_seq_map_common () +int +be_visitor_union_branch_public_reset_cs::visit_seq_map_common (be_type *) { // Implementation remains the same }And update the callers:
-return visit_seq_map_common (); +return visit_seq_map_common (node);TAO/tests/IDLv4/maps/test.idl (2)
17-31: Consider typedefs for long bounded-map lines & clarify “bounded” disclaimerThe comment says bounded maps aren’t actually bounded with
std::map; however, the IDL still exposes the bound in generated code. Consider:
- Adding a short note (IDL comment) next to each bounded map reminding future readers that the bound is only enforced at the IDL level, not in the runtime container.
- Introducing typedefs such as
typedef map<string, int32, 4> StringIntMap4;– this keeps the struct definition readable and avoids repetition.These are readability improvements; no functional impact.
37-44: Union default case missing
UnionWithMapenumerates explicit cases 0, 1, 2 but doesn’t declare adefault:label. Some compilers require an explicit default for completeness, especially once additional fields are added. Confirm this is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
TAO/NEWS(1 hunks)TAO/TAO_IDL/ast/ast_decl.cpp(3 hunks)TAO/TAO_IDL/be/be_codegen.cpp(1 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/private_ch.cpp(2 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/public_assign_cs.cpp(5 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp(4 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/public_ci.cpp(2 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp(2 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/public_reset_cs.cpp(2 hunks)TAO/TAO_IDL/be/be_visitor_union_branch/union_branch.h(2 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/private_ch.h(1 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/public_assign_cs.h(2 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ch.h(1 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ci.h(2 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/public_cs.h(1 hunks)TAO/TAO_IDL/be_include/be_visitor_union_branch/public_reset_cs.h(1 hunks)TAO/TAO_IDL/fe/idl.ypp(22 hunks)TAO/TAO_IDL/include/ast_decl.h(3 hunks)TAO/TAO_IDL/include/utl_idlist.h(1 hunks)TAO/tests/IDLv4/maps/main.cpp(2 hunks)TAO/tests/IDLv4/maps/test.idl(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- TAO/TAO_IDL/be/be_visitor_union_branch/union_branch.h
- TAO/TAO_IDL/be/be_codegen.cpp
- TAO/TAO_IDL/include/ast_decl.h
🚧 Files skipped from review as they are similar to previous changes (3)
- TAO/tests/IDLv4/maps/main.cpp
- TAO/TAO_IDL/ast/ast_decl.cpp
- TAO/NEWS
🧰 Additional context used
🧬 Code Graph Analysis (5)
TAO/TAO_IDL/be/be_visitor_union_branch/private_ch.cpp (1)
TAO/TAO_IDL/be_include/be_visitor_union_branch/private_ch.h (16)
node(33-33)node(35-35)node(36-36)node(37-37)node(38-38)node(39-39)node(40-40)node(41-41)node(42-42)node(43-43)node(44-44)node(45-45)node(46-46)node(47-47)node(48-48)node(49-49)
TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp (1)
TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp (4)
visit_seq_map_common(494-587)visit_seq_map_common(495-495)visit_map(488-492)visit_map(489-489)
TAO/TAO_IDL/be/be_visitor_union_branch/public_reset_cs.cpp (2)
TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp (6)
visit_sequence(102-106)visit_sequence(103-103)visit_seq_map_common(114-139)visit_seq_map_common(115-115)visit_map(108-112)visit_map(109-109)TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp (6)
visit_sequence(482-486)visit_sequence(483-483)visit_seq_map_common(494-587)visit_seq_map_common(495-495)visit_map(488-492)visit_map(489-489)
TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ch.h (1)
TAO/TAO_IDL/be_include/be_visitor_union_branch/private_ch.h (16)
node(33-33)node(35-35)node(36-36)node(37-37)node(38-38)node(39-39)node(40-40)node(41-41)node(42-42)node(43-43)node(44-44)node(45-45)node(46-46)node(47-47)node(48-48)node(49-49)
TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp (3)
TAO/TAO_IDL/be/be_visitor_union_branch/private_ch.cpp (4)
visit_seq_map_common(419-457)visit_seq_map_common(420-420)visit_map(413-417)visit_map(414-414)TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp (4)
visit_seq_map_common(114-139)visit_seq_map_common(115-115)visit_map(108-112)visit_map(109-109)TAO/TAO_IDL/be/be_visitor_union_branch/public_ci.cpp (4)
visit_seq_map_common(763-842)visit_seq_map_common(764-764)visit_map(757-761)visit_map(758-758)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: windows-2019 vs2019
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019WChar
- GitHub Check: VS2019Release64
- GitHub Check: macos-14-C++
- GitHub Check: macos-13-C++
- GitHub Check: alpine-3.18
🔇 Additional comments (33)
TAO/TAO_IDL/include/utl_idlist.h (1)
89-89: Good modernization with nullptr default parameterAdding a default value of
nullptrfor thecdrparameter simplifies constructor usage and follows modern C++ practices, making the code cleaner.TAO/TAO_IDL/be_include/be_visitor_union_branch/private_ch.h (2)
44-44: Support for map nodes added correctlyThe addition of the
visit_mapmethod properly extends the visitor pattern to handle map nodes, consistent with the PR's goal of improving IDL map support.
52-53: Good refactoring with common helper methodAdding a private helper method to consolidate common logic for sequences and maps follows the DRY principle and will make future maintenance easier.
TAO/TAO_IDL/be_include/be_visitor_union_branch/public_cs.h (2)
38-38: Consistently added map supportThe addition of the
visit_mapmethod correctly extends this visitor to handle map nodes, maintaining consistency with other visitor classes.
45-46: Good code reuse with common helper methodAdding the private helper method for sequence and map common logic promotes code reuse and maintainability across different visitor implementations.
TAO/TAO_IDL/be_include/be_visitor_union_branch/public_reset_cs.h (1)
43-43: Map support consistently implementedThe
visit_mapmethod properly extends this visitor class to handle map types in reset operations, aligning with the overall map support improvements.TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ch.h (2)
45-45: LGTM - Adding map type support for union branchesThe added visitor method properly extends the visitor pattern to handle map types in union branches, following the same pattern as other type visitors.
53-54: Good refactoring to unify sequence and map handlingAdding a private helper method to consolidate common logic for both sequence and map types is an excellent design choice that will reduce code duplication and improve maintainability.
TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ci.h (2)
43-43: LGTM - Consistent map supportThe added visitor method follows the established pattern for handling different types in union branches.
53-53: Good design pattern consistencyAdding the common helper method maintains consistency with the approach used in public_ch.h, ensuring uniform handling of sequence and map types across different visitors.
TAO/TAO_IDL/be_include/be_visitor_union_branch/public_assign_cs.h (2)
44-44: LGTM - Proper visitor extensionThe map visitor method correctly extends the visitor pattern to support map types in union branch assignment code generation.
54-54: Good code reuse approachThe common helper method for sequences and maps follows the same pattern as in other visitor classes, providing consistent code organization across the codebase.
TAO/TAO_IDL/be/be_visitor_union_branch/private_ch.cpp (4)
408-411: Good refactoring of existing sequence visitorRefactoring the existing sequence visitor to use the common helper method reduces code duplication and improves maintainability.
413-417: LGTM - Consistent map visitor implementationThe map visitor correctly follows the same pattern as the sequence visitor, delegating to the common helper method.
419-457: LGTM - Well-implemented common helper methodThe helper method follows the established pattern for visitor methods in this class. It correctly:
- Retrieves and validates context information
- Handles both sequence and map types uniformly
- Generates appropriate code for the private member variable
440-440: Good error message updateUpdating the error message to reflect the new method name ensures accurate error reporting.
TAO/TAO_IDL/be/be_visitor_union_branch/public_cs.cpp (3)
103-106: LGTM: Common method for sequence handling.The refactoring of
visit_sequenceto delegate to a common helper method is a good practice that will reduce code duplication.
108-112: LGTM: Addition of map support.Proper implementation of
visit_mapmethod to delegate to the common helper method, consistent with the sequence handling pattern.
132-132: LGTM: Updated error message.Correctly updated the error message to reflect the common method name.
TAO/TAO_IDL/be/be_visitor_union_branch/public_reset_cs.cpp (3)
389-392: LGTM: Refactored sequence handling.The
visit_sequencemethod has been properly refactored to delegate to the common helper method.
394-398: LGTM: Added map support.The
visit_mapmethod has been properly implemented to delegate to the common helper method.
412-412: LGTM: Updated error message.The error message has been correctly updated to reflect the common method name.
TAO/TAO_IDL/be/be_visitor_union_branch/public_assign_cs.cpp (5)
616-619: LGTM: Refactored sequence handling.The
visit_sequencemethod has been properly refactored to delegate to the common helper method.
621-625: LGTM: Added map support.The
visit_mapmethod has been properly implemented to delegate to the common helper method.
627-690: LGTM: Well-implemented common helper.The
visit_seq_map_commonimplementation correctly handles both copy construction and assignment cases for both sequence and map types.
650-650: LGTM: Updated error message.The error message has been correctly updated to reflect the common method name.
704-704: LGTM: Fixed error message method names.The error messages in other visitor methods have been corrected to reflect their actual method names instead of all incorrectly reporting "visit_array".
Also applies to: 751-751, 856-856
TAO/TAO_IDL/be/be_visitor_union_branch/public_ci.cpp (4)
752-755: LGTM: Refactored sequence handling.The
visit_sequencemethod has been properly refactored to delegate to the common helper method.
757-761: LGTM: Added map support.The
visit_mapmethod has been properly implemented to delegate to the common helper method.
763-842: LGTM: Well-implemented common helper.The
visit_seq_map_commonimplementation properly generates accessor methods for both sequence and map types, including both const and non-const versions.
786-786: LGTM: Updated error message.The error message has been correctly updated to reflect the common method name.
TAO/TAO_IDL/be/be_visitor_union_branch/public_ch.cpp (1)
483-486: Unified handling for sequences looks goodDelegating
visit_sequence()to the new helper reduces duplication and keeps this method minimal.TAO/tests/IDLv4/maps/test.idl (1)
31-34: Map-of-map key may be rejected by future IDL spec
map<TestIntStringMap, string>uses a map type as a key. While TAO currently accepts it (because C++std::mapprovides comparison operators), the OMG’s draft IDL4.3 (issue 70) discusses disallowing container types as keys. If compliance with the final spec is required later, this field might break. Keep this in mind for future portability.
It looks like it can't really handle |
Fixes for IDL maps (cherry picked from commit 8a02bf6) # Conflicts: # TAO/NEWS # TAO/TAO_IDL/be_include/be_visitor_union_branch/public_assign_cs.h # TAO/TAO_IDL/be_include/be_visitor_union_branch/public_ci.h # TAO/TAO_IDL/fe/idl.tab.cpp # TAO/TAO_IDL/fe/idl.ypp
3 participants
std::mapgenerated by the builtin backend doesn't enforce the bound.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation