ACE_TAO
ACE_TAO copied to clipboard
DOCGroup
Add maps to tao_idl
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.
https://github.com/objectcomputing/OpenDDS/issues/3236
@iguessthislldo, I believe the parsing is done, are you able to take a look at this.
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
Looks good so far!
I just remembered this. There's a C preprocessor file I added that states what features the IDL compiler supports. It should be updated to state it supports map. This will be important to check this in OpenDDS so OpenDDS can still be built with versions of TAO that don't support maps.
https://github.com/DOCGroup/ACE_TAO/blob/ee4008d6869a8796aec78a5e95f85cd20c862cdf/TAO/tao/idl_features.h#L67-L69
Change the 0 to TAO_IDL_IDL_VERSION >= 0x4000.
Also are you planning on working on the TAO C++ mapping for maps in this PR? I don't know if I have mentioned this yet, but if you're only interested in using C++11 mapping in OpenDDS, you are free to omit the C++ mapping here and only do it for C++11 in OpenDDS.
I'll make the change. And I'll be honest I'm doing this more for the fun and experience of it rather than for a particular purpose, so I'll do whatever makes this as complete as possible, so I can add it here first.
Any pointers for the C++ mapping, I did try to start, by trying to fix the errors when compiling the test idl with maps, however it did get out of hand quickly.
Also I'm not sure why but this line fails with ../TAO/tests/IDLv4/maps/test.idl", line 9: Some syntax error
typedef map<int32, TestStruct> TestStructMap;
And I'll be honest I'm doing this more for the fun and experience of it rather than for a particular purpose, so I'll do whatever makes this as complete as possible, so I can add it here first.
Alright, cool. I've definitely have projects like that. :smile: Just remember you're not obligated to do anything you don't want to. A minimal working version of this, especially from the TAO/CORBA perspective, is acceptable to me.
Any pointers for the C++ mapping, I did try to start, by trying to fix the errors when compiling the test idl with maps, however it did get out of hand quickly.
Not particularly unfortunately. I think I mentioned this in the check list but I'm less familiar with that side of things. If you're trying to implement the map from the XTypes spec and having trouble with it, it might just easier to start off with (or maybe permanently use) a std::map typedef. Other than that I'd imagine you might need to stub out a bunch of things TAO would want, like for serialization/marshaling. You'd probably need to stub them so they'll link, but you don't need to implement anything like that because OpenDDS doesn't use them and I wouldn't be able to really tell you how to implement them anyway since since CORBA doesn't have maps.
Also I'm not sure why but this line fails with ../TAO/tests/IDLv4/maps/test.idl", line 9: Some syntax error
typedef map<int32, TestStruct> TestStructMap;
That's hard to say. Maybe something is wrong with the recursive checks? I'd imagine that would have it's own messages though. I will try to find some time today to build your branch and see if I can see the problem. In the mean time if it helps there's a --bison-trace option in tao_idl for parsing to see what bison is doing.
So I'm pretty sure I got the code generation for maps working, it's just an std::map typedef. I'm not sure exactly where to #include <map> right now it just includes it all the time everywhere, however should maps act the same as sequence and only generate std::map with -Gstl (then error otherwise instead of a custom map).
So I'm pretty sure I got the code generation for maps working, it's just an std::map typedef. I'm not sure exactly where to
#include <map>right now it just includes it all the time everywhere, however should maps act the same as sequence and only generate std::map with -Gstl (then error otherwise instead of a custom map).
In opendds_idl there's a data structure (based on std::set to eliminate duplicates) that keeps track of what should be included in the generated code. Whenever the code generation pass emits a use of a standard library type, it adds an entry to this data structure with the appropriate standard library header. At the end of code generation passes it writes the required headers into the actual output file.
See https://github.com/DOCGroup/ACE_TAO/blob/587bacedd8b09849a25c3fa368b5fc315fe45ee6/TAO/TAO_IDL/be/be_codegen.cpp#L3027 for how this is done for other types that have conditional includes
I think this is done and ready for proper review, I've already started messing with this branch in OpenDDS
There are some checks for illegal IDL for maps, would recommend to add also a test to validate that illegal map definitions are correctly rejected without a crash of tao_idl, see for example Bug_3845_Regression for a run_test.pl
It's been quite a while since I opened this and I know it's a fairly massive PR, but I did just want to get some clarity on what's needed for this to be merged, to collect all the remaining tasks in one place.
Functionality should all be there (to the point I've started on the OpenDDS implementation using it). There have been quite a few copy-paste typos (comments with sequence instead map) that I've tried searching for but people have somehow found more. Is there anything else
I only did a brief review, before a merge someone has to do a detailed review and preferable do a full build of the full distribution to make sure nothing breaks (github actions only compiles the core). At some point someone has to merge it, but that person also takes the responsibility to keep an eye on all build/tests and coordinate any breakage. At this moment I don't have enough time to review this all just for free.
That's fine, take your guys' time, I just wanted to make sure it wasn't blocked on me. I appreciate everything done so far.
Maybe someone else has time to review and merge at some point, not sure, I am pretty busy right now
Maybe someone else has time to review and merge at some point, not sure, I am pretty busy right now
Between @iguessthislldo and myself (and anyone else who'd like to contribute) we'll get it reviewed
New warning:
/home/johnny/ACE/trunk/TAO/TAO_IDL/be/be_visitor_field/cdr_op_ch.cpp: In member function ‘virtual int be_visitor_field_cdr_op_ch::visit_map(be_map*)’:
/home/johnny/ACE/trunk/TAO/TAO_IDL/be/be_visitor_field/cdr_op_ch.cpp:151:48: warning: unused parameter ‘node’ [-Wunused-parameter]
151 | be_visitor_field_cdr_op_ch::visit_map (be_map *node)
| ~~~~~~~~^~~~
2022-10-05T07:29:22.2971107Z /home/runner/work/ACE_TAO/ACE_TAO/TAO/TAO_IDL/be/be_visitor_map/cdr_op_cs.cpp:26:1: warning: unused parameter ‘node’ [-Wunused-parameter]
2022-10-05T07:29:22.2971801Z be_visitor_map_cdr_op_cs::visit_map (be_map *node)
2022-10-05T07:29:22.2972112Z ^
2022-10-05T07:37:26.4155029Z /home/runner/work/ACE_TAO/ACE_TAO/TAO/orbsvcs/IFR_Service/ifr_adding_visitor.cpp:2305:1: warning: unused parameter ‘node’ [-Wunused-parameter]
2022-10-05T07:37:26.4229668Z ifr_adding_visitor::visit_map (AST_Map *node)
Compiling new IDLv4 map test gives a lot of warnings:
GNUmakefile: /home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/GNUmakefile.IDLv4_maps MAKEFLAGS=w
/home/johnny/ACE/trunk/ACE/bin/tao_idl -Wb,pre_include=ace/pre.h -Wb,post_include=ace/post.h -I ../../.. -Sa -St --idl-version 4 test.idl
g++ -Wnon-virtual-dtor -fvisibility=hidden -fvisibility-inlines-hidden -ggdb -pthread -fno-strict-aliasing -Wall -Wextra -Wpointer-arith -pipe -D_GNU_SOURCE -I/home/johnny/ACE/trunk/ACE -DACE_NO_INLINE -I/home/johnny/ACE/trunk/ACE -I../../.. -DACE_HAS_IPV6 -c -o .obj/main.o /home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp: In function ‘int main(int, ACE_TCHAR**)’:
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp:24:26: warning: ISO C++ forbids converting a string constant to ‘std::map<int, char*>::mapped_type’ {aka ‘char*’} [-Wwrite-strings]
24 | testIntStringMap[42] = "Hello World!";
| ^~~~~~~~~~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp:30:29: warning: ISO C++ forbids converting a string constant to ‘std::map<char*, TestStruct>::key_type’ {aka ‘char*’} [-Wwrite-strings]
30 | testData.stringStructsMap["test"] = testStruct;
| ^~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp:31:30: warning: ISO C++ forbids converting a string constant to ‘std::map<char*, TestStructSeq>::key_type’ {aka ‘char*’} [-Wwrite-strings]
31 | testData.stringSequenceMap["test"] = testStructSeq;
| ^~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp:32:25: warning: ISO C++ forbids converting a string constant to ‘std::map<char*, std::map<int, TestStruct> >::key_type’ {aka ‘char*’} [-Wwrite-strings]
32 | testData.stringMapMap["test"] = testStructMap;
| ^~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/main.cpp:33:45: warning: ISO C++ forbids converting a string constant to ‘std::map<std::map<int, char*>, char*>::mapped_type’ {aka ‘char*’} [-Wwrite-strings]
33 | testData.mapStringMap[testIntStringMap] = "Hello World";
| ^~~~~~~~~~~~~
g++ -Wnon-virtual-dtor -fvisibility=hidden -fvisibility-inlines-hidden -ggdb -pthread -fno-strict-aliasing -Wall -Wextra -Wpointer-arith -pipe -D_GNU_SOURCE -I/home/johnny/ACE/trunk/ACE -DACE_NO_INLINE -I/home/johnny/ACE/trunk/ACE -I../../.. -DACE_HAS_IPV6 -c -o .obj/testC.o /home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp: In function ‘CORBA::Boolean operator<<(TAO_OutputCDR&, const RecurseStruct&)’:
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:119:20: warning: unused parameter ‘strm’ [-Wunused-parameter]
119 | TAO_OutputCDR &strm,
| ~~~~~~~~~~~~~~~^~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:120:26: warning: unused parameter ‘_tao_aggregate’ [-Wunused-parameter]
120 | const RecurseStruct &_tao_aggregate)
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp: In function ‘CORBA::Boolean operator>>(TAO_InputCDR&, RecurseStruct&)’:
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:128:19: warning: unused parameter ‘strm’ [-Wunused-parameter]
128 | TAO_InputCDR &strm,
| ~~~~~~~~~~~~~~^~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:129:20: warning: unused parameter ‘_tao_aggregate’ [-Wunused-parameter]
129 | RecurseStruct &_tao_aggregate)
| ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp: In function ‘CORBA::Boolean operator<<(TAO_OutputCDR&, const DataStruct&)’:
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:199:20: warning: unused parameter ‘strm’ [-Wunused-parameter]
199 | TAO_OutputCDR &strm,
| ~~~~~~~~~~~~~~~^~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:200:23: warning: unused parameter ‘_tao_aggregate’ [-Wunused-parameter]
200 | const DataStruct &_tao_aggregate)
| ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp: In function ‘CORBA::Boolean operator>>(TAO_InputCDR&, DataStruct&)’:
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:215:19: warning: unused parameter ‘strm’ [-Wunused-parameter]
215 | TAO_InputCDR &strm,
| ~~~~~~~~~~~~~~^~~~
/home/johnny/ACE/trunk/TAO/tests/IDLv4/maps/testC.cpp:216:17: warning: unused parameter ‘_tao_aggregate’ [-Wunused-parameter]
216 | DataStruct &_tao_aggregate)
| ~~~~~~~~~~~~^~~~~~~~~~~~~~
Why is in the generated testC.cpp for the new maps there also typedef std::map, isn't enough to just have that in the header, for example IntRecursedMap
No need for a space between std::map< and the type.
Instead of generating just true in the operator<< and >>, why not throw an exception CORBA::NO_IMPLEMENT ()? That will clearly show that a map can't be send yet.
Instead of typedef generate using, just as we do for a lot of other things, that is preferred with C++11
I think we should add tests/IDLv4 as part of this PR also to the github actions (see .github directory)
2022-10-05T07:29:22.2971107Z /home/runner/work/ACE_TAO/ACE_TAO/TAO/TAO_IDL/be/be_visitor_map/cdr_op_cs.cpp:26:1: warning: unused parameter ‘node’ [-Wunused-parameter] 2022-10-05T07:29:22.2971801Z be_visitor_map_cdr_op_cs::visit_map (be_map *node) 2022-10-05T07:29:22.2972112Z ^2022-10-05T07:37:26.4155029Z /home/runner/work/ACE_TAO/ACE_TAO/TAO/orbsvcs/IFR_Service/ifr_adding_visitor.cpp:2305:1: warning: unused parameter ‘node’ [-Wunused-parameter] 2022-10-05T07:37:26.4229668Z ifr_adding_visitor::visit_map (AST_Map *node)
This actually brings up another thing, on my own review I noticed I forgot to change the visit_map in be_visitor_field/field.ch.cpp. I'm not entirely sure what a lot of the different visitors are meant to do, I can squash the warnings (just be removing the variable) but not sure what those versions of visitor should be doing
Also this warning
ISO C++ forbids converting a string constant to ‘std::map<char*, std::map<int, TestStruct> >::key_type’ {aka ‘char*’} [-Wwrite-strings]
If I remember correctly I'm using the wrong string type, I had trouble extracting the proper type from the key and value nodes
Also this warning
ISO C++ forbids converting a string constant to ‘std::map<char*, std::map<int, TestStruct> >::key_type’ {aka ‘char*’} [-Wwrite-strings]If I remember correctly I'm using the wrong string type, I had trouble extracting the proper type from the key and value nodes
It should be similar to strings that appear in structs, or sequences. Some sort of manager object (maybe reusing one of those) would be the actual key_type or mapped_type for std::map.
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
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
Is it possible to split adding a bounded map into a separate PR as this one is huge as it is?
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.
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.