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

CastXML integration #66

Merged
merged 67 commits into from
Mar 29, 2016
Merged

CastXML integration #66

merged 67 commits into from
Mar 29, 2016

Conversation

goldhoorn
Copy link
Member

I started to try to integrate castxml, but it seems that this is NOT a drop-in-replacement.

I currently generate all outputs from gccxml and castxml (castxml is active)
the calls to castxml wen trught, but the gnerated registry is empty.
Someone has to dig into this why the castxml output is not processed correctly.

This is work for either
@doudou, or (Team 8 responsibility) @jmachowinski @saarnold or @jhidalgocarrio

@goldhoorn goldhoorn changed the title Integrate CastXML DO NOT MERGE: Integrate CastXML Aug 18, 2015
@doudou
Copy link
Contributor

doudou commented Aug 18, 2015

Hey, thanks.

You forgot to change the castxml handler to use .hpp files ... (I say "forgot" as you did it in the preprocess method)

@doudou
Copy link
Contributor

doudou commented Aug 18, 2015

Still not working with that change, though.

@goldhoorn
Copy link
Member Author

This is not needed because the file is not processed later. Only the string is returned .

However semms a bit of work. I can provide tomorrow the preprocessed files.

@doudou
Copy link
Contributor

doudou commented Aug 18, 2015

I've brought the stuff up to a point where it looks promising. Basically, the "major" change is that the 'demangled' field has disappeared. The thing is, it was not always there and so using it was making the code that much more complicated.

Code's not clean either. Let's get it to work first.

@goldhoorn
Copy link
Member Author

Please keep ind mind that this change is also needed (sorry i forgot to push yesterday):
orocos-toolchain/orogen#64

@doudou
Copy link
Contributor

doudou commented Aug 19, 2015

Please keep ind mind that this change is also needed (sorry i forgot to push yesterday)

Actually, it was not (but I merged it anyways, forgetting about that)

The performance optimization of putting all includes in a single temporary file fails with castxml (the type-to-file mappings are not reported properly when doing that). See my comment above.

@goldhoorn
Copy link
Member Author

The all in one file causes also this side-problem (or at least would solve it)
orocos-toolchain/orogen#50

@goldhoorn
Copy link
Member Author

Ok i spend a bit work in here, the current state is that the importer now roughly works.
I hit a few bugs in typelib (specially the return string <-> nil) problem.

However the current state is that the base/orogen/std could be generated successfull (as far i see).
The problem with base/orogen/types is now definitily the same one as in orocos-toolchain/orogen#50 it seems the headers are reported in a different order by castxml.

However, the orogen bug must be solved before any further development can be done here.
The bug occus here even on a (not-export-all-typekits) setup.

@goldhoorn
Copy link
Member Author

@marvin2k (cc)

@goldhoorn
Copy link
Member Author

For the sake of clarity: i will not work on this include bug if this header resolution is fixed i might have time to check castxml itself and help for a clear integration. But this special bug needs to be solved by someone else.

@@ -695,6 +647,7 @@ def self.parse_cxx_documentation_before(lines, line)
def load(required_files, xml)
@registry = Typelib::Registry.new
Typelib::Registry.add_standard_cxx_types(registry)
registry.alias('/std/basic_string</char>', '/std/string')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be moved into add_standard_cxx_types? It left my memory why this manual alias is not needed for the gccxml-case...

@doudou
Copy link
Contributor

doudou commented Oct 29, 2015

FYI, I've cleaned up the patches, and -- much more importantly -- moved the clang test suite to the common parts and apply it to gccxml, clang and castxml. Currently gccxml/castxml pass most of them (one does not pass because it depends on Eigen ... @marvin2k could you remove the dependency ?). The other one I've not looked into it yet.

What is tested so far is the ability to define the types properly. Metadata is not tested, but it will (especially given how important it is to the orogen side)

I'll from now on have a policy that every single bugfix in the C++ importer should have a corresponding test added.

@goldhoorn
Copy link
Member Author

@doudou: do you have a general concept (in mind) how we handle the cxx11 abi's?
Is really ignoring (as here: 64a309d) the way to go?

@marvin2k
Copy link
Contributor

Hm, I added the eigen dependency to check the opaque mechanism. Removed them now. I don't know how to run the tests now, so testing this commit is left for the curious among us ;-)

Thanks for attacking this castxml problem, this removes the need for a lot of long-time work on the clang importer!

Ah, I did not edit the generated templates.tlb as it will be generated automatically by running the testsuite?

@doudou
Copy link
Contributor

doudou commented Oct 29, 2015

Ah, I did not edit the generated templates.tlb as it will be generated automatically by running the testsuite?

Actually not, the tlb files are used as expected result and compared with the importer result.

@marvin2k
Copy link
Contributor

So how are the expected results generated then? By hand? In the cmake-based testing the tlb's were generated in the build-dir and had to be copied manually into the src-folder.

@doudou
Copy link
Contributor

doudou commented Oct 29, 2015

So how are the expected results generated then? By hand? In the cmake-based testing the tlb's were generated in the build-dir and had to be copied manually into the src-folder.

That's what I had in mind to add new "expected" file. Generate it, check it by hand to make sure it matches and then add it to the test folder.

@marvin2k
Copy link
Contributor

The CMakeList with the target is "gone". I thought this is somehow replaced by minitest?

@doudou
Copy link
Contributor

doudou commented Oct 29, 2015

I'm trying to iron out a process here to

  • easily run a ruby test (currently easy to run the whole test suite, but a pain to run a single test)
  • save the generated tlb files in the build directory

I'll come back to you

@marvin2k
Copy link
Contributor

in shell-speak:

./build/tools/typelib-clang-tlb-importer/typelib-clang-tlb-importer test/ruby/cxx_import_tests/templates.hh -opaquePath=test/ruby/cxx_import_tests/templates.opaques -tlbSavePath=/tmp/templates.tlb
# inspect inspect inspect...
cp /tmp/templates.tlb test/ruby/cxx_import_tests/templates.tlb

@doudou
Copy link
Contributor

doudou commented Oct 29, 2015

in shell-speak:

This assumes that you have a clang importer.

@doudou
Copy link
Contributor

doudou commented Nov 9, 2015

Rebased and updated things. The castxml and gccxml importers now pass the test suite

One major problem is that castxml does not output the offset of base classes (meaning, it will break for multiple inheritance). I've added a ticket to castxml about that (CastXML/CastXML#34)

@doudou
Copy link
Contributor

doudou commented Nov 9, 2015

To run the tests, there's a runner script in build/test/ruby. Just cd in there and run

 ./runner
 ./runner test_cxx_gccxml.rb
 ./runner test_cxx_gccxml.rb -n /test_name_filter/

To generate the tlbs, run

 ./cxx_tlbgen name_of_test_file.hh

@hauptmech
Copy link

I think I need castxml integrated to build rock with gcc5. How's this going?

doudou and others added 26 commits March 17, 2016 11:34
One can add a .tlb.IMPORTER_NAME file that is specific to the importer, in which
case the test verifies that the loaded registry has all tests for the common
registry, and complete equivalence for the loader-specific registry

This is because the clang importer currently does not support pointers, but
gccxml/castxml does, and I don't want to make it a strong requirement just yet.
Some tests where still selecting their loader based on
the TYPELIB_CXX_LOADER envvar, which goes contrary to what we
actually want.
The ones starting with __
After this commit, both gccxml and castxml pass the suite
…rs in template

Negative integers (e.g. -2) are represented as -0x000000N
(e.g. -2 is -0x000000002)
One should add it through the TYPELIB_CASTXML_DEFAULT_OPTIONS environment
variable
clang/castxml can handle the vectorization instructions just fine
@doudou
Copy link
Contributor

doudou commented Mar 29, 2016

Given the 98% lack of review of PRs (important or otherwise) from anyone to rock-core in the last year or so, I merge this.

@doudou doudou merged commit 2ab2375 into master Mar 29, 2016
@doudou doudou deleted the castxml branch March 29, 2016 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants