orogen cannot handle "C style" typedef'ed structs correctly

[g-arjones]

I'm almost sure this is a regression because I (we? :P) used this before and I don't remember having to work around it...

Given the definition:

typedef struct _Foobar {
    int dummy_field;
} Foobar;

It seems Typelib does not generate definitions for _Foobar but still creates convertion methods for it:

.orogen/typekit/transports/corba/Convertions.cpp:17:34: error: ‘_Foobar’ is not a member of ‘orogen::Corba’
bool toCORBA( orogen::Corba::_Foobar& corba, _Foobar const& value );
^~~~~~~
.orogen/typekit/transports/corba/Convertions.cpp:17:34: note: suggested alternative: ‘Foobar’
bool toCORBA( orogen::Corba::_Foobar& corba, _Foobar const& value );
^~~~~~~
Foobar
.orogen/typekit/transports/corba/Convertions.cpp:17:43: error: ‘corba’ was not declared in this scope
bool toCORBA( orogen::Corba::_Foobar& corba, _Foobar const& value );
^~~~~

Possible regression? I think I have used this before..

[doudou]

I honestly don't know.

[g-arjones]

Could you please provide some pointers? This is a huge setback for me and debugging orogen/typelib is proving really painful so far

[doudou]

Could you please provide some pointers?

A first workaround I would see is to try and separate the typedef and the struct, which really should be working.

For debugging, the first step is to look into the generated .tlb file, that you can find in .orogen/typekit. This is the XML representation of the types that Typelib parsed. In principle, I would expect to find a _Foobar compound type aliased to Foobar.

If that's not the case, we can get it from there (i.e. have to look into typelib's gccxml parser, and in particular see whether castxml's output is different in the "separate" and "join" cases).

If the tlb file has the expected pattern, then orogen is probably to blame, but I don't understand why it would mis-behave on this particular pattern.

[g-arjones]

Thanks.. I will look into debugging, this is an auto generated code so I can't really work on the first suggestion...

[doudou]

Don't hesitate if you got questions or need more pointers. These two are indeed not the most understandable codebases ...

[g-arjones]

Don't hesitate

Definitely. Appreciate the help, thanks.

[doudou]

To improve your workflow, if typelib is indeed the problem, there is a list of unit tests for the C++ importer in test/ruby/cxx_import_tests. You've got to drop a .hh and an expected .tlb and then can run ./runner test_cxx_castxml.rb -n=/test_cxx_common/ from within the build dir

acd -b .
cd test/ruby
./runner test_cxx_castxml.rb -v -n=/test_cxx_common/

The cxx_tlbgen script (also in the build dir) creates a tlb from a .hh file (to start). Run cxx_tlbgen name_of_file.hh after having dropped the .hh into test/ruby/cxx_import_tests.

Note that there is already a typedefs.hh in there that covers a bunch of cases, but not yours.

[g-arjones]

I've tried your first suggestion just to try to pinpoint the problem and found that:

1. Both

   typedef struct _Foobar {
   int dummy_field;
   } Foobar;

and

struct _Foobar {
    int dummy_field;
};

typedef _Foobar Foobar;

Give me exactly the same, which is:

  <compound name='/_Foobar' size='4'>
    <field name='dummy_field' offset='0' type='/int32_t'>
      <metadata key='source_file_line'><![CDATA[/home/docker/dev/install/drivers/dummy_pkg/include/dummy_pkg/test.hpp:2]]></metadata>
    </field>
    <metadata key='cxxname'><![CDATA[::_Foobar]]></metadata>
    <metadata key='orogen_defining_typekits'><![CDATA[dummy_pkg]]></metadata>
    <metadata key='orogen_exporting_typekits'><![CDATA[dummy_pkg]]></metadata>
    <metadata key='orogen_include'><![CDATA[dummy_pkg:dummy_pkg/test.hpp]]></metadata>
    <metadata key='source_file_line'><![CDATA[/home/docker/dev/install/drivers/dummy_pkg/include/dummy_pkg/test.hpp:1]]></metadata>
  </compound>
  <alias name='/int' source='/int32_t'/>
  <alias name='/int signed' source='/int32_t'/>
  <alias name='/signed' source='/int32_t'/>
  <alias name='/signed int' source='/int32_t'/>
  <alias name='/Foobar' source='/_Foobar'/>

Which is expected, since they are really the same thing. OroGen fails with both.

2. Surprisingly enough (and not sure how relevant this is), changing the definition to an anonymous struct:

   typedef struct {
   int dummy_field
   } Foobar;

Gives me nothing in the .tlb file.

3. A named struct with no typedefs works as expected:

   struct Foobar {
   int dummy_field;
   };

Gives me:

  <compound name='/Foobar' size='4'>
    <field name='dummy_field' offset='0' type='/int32_t'>
      <metadata key='source_file_line'><![CDATA[/home/docker/dev/install/drivers/dummy_pkg/include/dummy_pkg/test.hpp:2]]></metadata>

    </field>
    <metadata key='cxxname'><![CDATA[::Foobar]]></metadata>
    <metadata key='orogen_defining_typekits'><![CDATA[dummy_pkg]]></metadata>
    <metadata key='orogen_exporting_typekits'><![CDATA[dummy_pkg]]></metadata>
    <metadata key='orogen_include'><![CDATA[dummy_pkg:dummy_pkg/test.hpp]]></metadata>
    <metadata key='source_file_line'><![CDATA[/home/docker/dev/install/drivers/dummy_pkg/include/dummy_pkg/test.hpp:1]]></metadata>
  </compound>

The only difference being the lack of alias. And the oroGen generated code builds fine.

Since (2) (the anonymous struct) is probably a limitation in Typelib it seems that the Foobar alias is breaking oroGen for some reason. Is that what it looks like to you? Thoughts?

[g-arjones]

So, the problem is neither orogen nor typelib. omniidl removes the leading underscore from symbols in the generated files. This means that

struct _Foobar {
    int dummy_field;
};

breaks as well. I'm trying to figure out why they do that and/or if there's a workaround.

[g-arjones]

How would you feel about doing that in oroGen as well (turning _Foobar into Foobar)? Maybe as an option...

[doudou]

How would you feel about doing that in oroGen as well (turning _Foobar into Foobar)? Maybe as an option...

Since this is a CORBA limitation, the CORBA generator could remove the leading underscore, with a sanity check to avoid clashes. That would seem a reasonable place to do it (so, in orogen but only for CORBA).

Another possibility would be to allow to specify that orogen should use an alias as the type name, ("rewriting" the registry), which would be more generally useful. It would still have to check that the imported type defines the alias in the first place (or things won't compile).

invert_alias "Foobar"

[g-arjones]

the CORBA generator could remove the leading underscore

If the "CORBA generator" is the thing that generates the IDL, that's what I had in mind and seemed reasonable. CORBA's specs have a few other constraints as well (i.e symbols are case-insensitive)

with a sanity check to avoid clashes

omniidl outputs a very clear error message about that. Do we really need orogen doing the same?

Another possibility would be to allow to specify that orogen should use an alias as the type name

Not sure I get this one.

[g-arjones]

invert_alias "Foobar"

Ah, got it now. Not sure which one is best. Both sound easy to implement (the test suite is a nightmare though)

[doudou]

Not sure which one is best

The only plus for the invert_alias is that it removes the annoying _Factor name. But I believe the IDL generation fix would be a lot easier/quicker. Changing type names in a Typelib registry will touch a lot more code.

the test suite is a nightmare though

How so ?

You're not supposed to extend on the code generation tests. If you have codegen tests, they should go as a separate package in the rock build tests (https://github.com/rock-core/rock.build-tests-buildconf). See for instance https://github.com/rock-core/rock.build-tests-buildconf/blob/master/rock.build-tests/packages.autobuild#L40

If the test is only build/no build, then you're done. If you need some post-build tests, add them to https://github.com/rock-core/rock.build-tests-buildconf/blob/master/test/orogen_test.rb

[g-arjones]

You're not supposed to extend on the code generation tests

I think it would be valuable to have those (unit tests makes maintaining things easier for developers not that familiar with the codebase) but I'll follow your advice and add only build tests.

[doudou]

The build test in this case is testing that whatever orogen is generating is accepted by the omniidl compiler which is then accepted by the C++ compiler. I don't see anything but an integration test.

However if you want to test a particular method that does not need to go through a codegen & build stage, do add it to the test suite. But if you need to shell out to anything else, don't.

In this particular case, the fix will probably end up in typelib's idl exporter, in which case I do expect you to implement a unit test there and a build test for regression.

[g-arjones]

Changing the IDL exporter has no effect, actually. Removing the leading underscores from the names in the IDL file is pointless, the C++ code that omniidl generates will be the same (we just removed something that omniidl itself would have ignored anyway).

I think we have to either:

1. Modify typelib so that it removes the leading underscores from compound and field names in the .tlb files (will have to deal with aliases in the process) since that's what oroGen uses to generate its C++ code, or;
2. Make orogen aware that omniidl will remove leading underscores from names and do that as well. Probably in the corba marshaller but maybe in other places as well.

I think (2) is easier.

Thoughts?

[doudou]

The cleanest implementation would be to have orogen's corba marshaller normalize a copy of the type registry where the leading underscores have been removed. But that's quite a bit of work.

My second best pick would be (2) and to modify typelib's IDL generator to remove the leading underscores.