Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IDL4 map support to TAO #1842

Merged
merged 108 commits into from
Aug 22, 2023
Merged

Add IDL4 map support to TAO #1842

merged 108 commits into from
Aug 22, 2023

Conversation

tmayoff
Copy link
Contributor

@tmayoff tmayoff commented May 23, 2022

This a WIP PR for adding maps to tao_idl. Currently I have the flex/bison and ast generation working (although a tad messy), as well as a simple test to verify it doesn't crash.

OpenDDS/OpenDDS#3236

@tmayoff
Copy link
Contributor Author

tmayoff commented May 26, 2022

@iguessthislldo, I believe the parsing is done, are you able to take a look at this.

Copy link
Member

@iguessthislldo iguessthislldo left a comment

Choose a reason for hiding this comment

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

I'm not sure if including the MPC submodule is intentional or not. If it is, I think it would better for a separate PR.

TAO/TAO_IDL/ast/ast_generator.cpp Outdated Show resolved Hide resolved
TAO/tests/IDLv4/maps/main.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/include/ast_map.h Outdated Show resolved Hide resolved
TAO/TAO_IDL/fe/idl.ypp Outdated Show resolved Hide resolved
TAO/TAO_IDL/include/ast_map.h Outdated Show resolved Hide resolved
TAO/tests/IDLv4/maps/.gitignore Outdated Show resolved Hide resolved
TAO/tests/IDLv4/maps/test.idl Show resolved Hide resolved
@tmayoff
Copy link
Contributor Author

tmayoff commented Jun 3, 2022

I'm not sure if including the MPC submodule is intentional or not. If it is, I think it would better for a separate PR.

That was not intentional, will remove it, thanks for the review

TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/fe/idl.ypp Outdated Show resolved Hide resolved
TAO/TAO_IDL/fe/idl.ypp Outdated Show resolved Hide resolved
TAO/TAO_IDL/fe/idl.ypp Show resolved Hide resolved
TAO/TAO_IDL/ast/ast_map.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/include/ast_map.h Outdated Show resolved Hide resolved
TAO/tests/IDLv4/maps/test.idl Outdated Show resolved Hide resolved
Copy link
Member

@jwillemsen jwillemsen left a comment

Choose a reason for hiding this comment

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

Why remove a legal IDL test case? What problem do some compilers have? When this is a real issue for you, maybe leave it commented out, when all compilers work again you can remove the comment (add a comment why you disable it)

Copy link
Member

@jwillemsen jwillemsen left a comment

Choose a reason for hiding this comment

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

I see now that IDL 4.2 allows any type as key type, which looks to be not correct, reported an issue for that, not all types can be compared

@tmayoff
Copy link
Contributor Author

tmayoff commented Mar 21, 2023

Sorry for the lack of details, was mainly testing and setting my dev environment back up. But yes, we can create maps with structs as keys, but it'll fail to compile when trying to use them for anything due to a lack of comparison operators

@jwillemsen jwillemsen changed the title Add maps to tao_idl Add IDL4 map support to TAO Jul 14, 2023
@jwillemsen
Copy link
Member

See https://github.com/RemedyIT/taox11/blob/master/tao/x11/bounded_map_t.h for some work in progress for a bounded map type. Just compilation of declaring a map, haven't tested at all, this is taken from our bounded sequence support

@tmayoff
Copy link
Contributor Author

tmayoff commented Aug 8, 2023

Is it possible to split adding a bounded map into a separate PR as this one is huge as it is?

@iguessthislldo
Copy link
Member

I would okay with that. I was already rereviewing this because we'd like to get this in before OpenDDS 4 and I don't want to make things harder.

@jwillemsen
Copy link
Member

Is it possible to split adding a bounded map into a separate PR as this one is huge as it is?

No problem with that, just wanted to let you know that I am working on IDL4 to C++11 as part of TAOX11. The parsing support in RIDL is much easier compared to TAO_IDL.

Doing this step by step, for marshaling I want to look at how TAO now marshals a sequence of a struct with a key and value and do something similar for a map.

@jwillemsen
Copy link
Member

jwillemsen commented Aug 9, 2023

I managed to add ostream and CDR support to TAOX11 for a IDL4 map (bounded and unbounded). Just created a PR with the CDR support and a basic test, a CORBA client and server who can exchange a IDL4 map remotely, see RemedyIT/taox11#308

Copy link
Member

@iguessthislldo iguessthislldo left a comment

Choose a reason for hiding this comment

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

Apologies it took so long to come back to this. Disregarding bounded maps, I think everything looks okay for now. Only need to address two unused parameter names.

TAO/TAO_IDL/be/be_visitor_map/map_cs.cpp Outdated Show resolved Hide resolved
TAO/orbsvcs/IFR_Service/ifr_adding_visitor.cpp Outdated Show resolved Hide resolved
tmayoff and others added 2 commits August 18, 2023 13:44
Co-authored-by: Fred Hornsey <fred@hornsey.us>
Co-authored-by: Fred Hornsey <fred@hornsey.us>
@iguessthislldo iguessthislldo merged commit 6c64cee into DOCGroup:master Aug 22, 2023
42 checks passed
@tmayoff tmayoff deleted the maps branch August 22, 2023 18:54
@jwillemsen
Copy link
Member

Can you add this enhancement to the TAO/NEWS file?

@tmayoff
Copy link
Contributor Author

tmayoff commented Aug 23, 2023

Small new PR here: #2098

@jwillemsen
Copy link
Member

@tmayoff New coverity issue, can you make a PR?

________________________________________________________________________________________________________
*** CID 1567665:  Uninitialized members  (UNINIT_CTOR)
/DOC_ROOT/TAO/TAO_IDL/be/be_type.cpp: 47 in be_type::be_type(AST_Decl::NodeType, UTL_IdList *)()
41         seen_in_operation_ (false)
42     {
43       if (n != nullptr)
44         {
45           this->gen_fwd_helper_name ();
46         }
>>>     CID 1567665:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member "seen_in_map_" is not initialized in this constructor nor in any functions that it calls.
47     }
48     
49     be_type::~be_type ()
50     {
51     }
52     

@tmayoff
Copy link
Contributor Author

tmayoff commented Aug 28, 2023

#2103

@jwillemsen
Copy link
Member

Test should have been added to the test lst file, see #2104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants