Skip to content

Java: C++11 strong enum and various fixes#215

Merged
wsfulton merged 10 commits into
swig:masterfrom
alexey-pelykh:cpp11_strongly_typed_enums__direct_inject_in_java
Dec 7, 2014
Merged

Java: C++11 strong enum and various fixes#215
wsfulton merged 10 commits into
swig:masterfrom
alexey-pelykh:cpp11_strongly_typed_enums__direct_inject_in_java

Conversation

@alexey-pelykh

Copy link
Copy Markdown
Contributor

Take into account strongly-typed values scope name

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

This should fix the #147

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

This doesn't fix case

namespace NS1
{
    class Class1
    {
    public:
        enum class Enum1
        {
            Value1
        }
    };
}

In C++ wrappers swig still generates NS1::Class1::Value1 instead of NS1::Class1::Enum1::Value1,
but I hope to fix that tomorrow.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Just realized that it should be fixed differently

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Ok, now it's working as needed for Java. Unfortunately I have no reliable way to test how this affects other languages.

@alexey-pelykh alexey-pelykh reopened this Aug 30, 2014
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Also, somewhere there in SWIG there's an issue that emits

*(OsmAnd::Bitmask< enum OsmAnd::MapRendererStateChange,unsigned int >::BitmaskT **)&jresult = result; 

instead of

*(OsmAnd::Bitmask< enum class OsmAnd::MapRendererStateChange,unsigned int >::BitmaskT **)&jresult = result; 

Where that can be located?

…for inner classes '$' should be used as separator instead of '/'
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

When JNI descriptor (e.g. for directors) is emitted for inner class, '/' was used instead of '$' - fixed.

@alexey-pelykh alexey-pelykh changed the title C++11 strongly-typed enums fix for Java only Java: C++11 strong enum and various fixes Sep 3, 2014
@wsfulton

Copy link
Copy Markdown
Member

I'm looking at this patch now, thanks for submitting it. I can't replicate the problem requiring https://github.com/alexey-pelykh/swig/commit/e7db081d5e175024f1d5baa9ac028474958a2542. What is the use case? We'll need to add it as a runtime testcase.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

If I remember correctly, it's for the case when class marked as director is inside other class in c++. When this happened, generated native code that resolved methods from the Java side failed since their JNI descriptor was generated as for Java name ("/" was used instead of "$" since "." got replaced)

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

I have tested these patches on our project https://github.com/osmandapp/OsmAnd-core using this script to generate wrapper https://github.com/osmandapp/OsmAnd-core/blob/master/wrappers/java/generate.sh

There are still issues with generated wrappers but so far I've made a workaround using SWIG_OMIT

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

One of the issues I haven't fixed - in wrappers a "enum class" forward declaration is still emitted as "enum" in directors if I remember correctly

@wsfulton

Copy link
Copy Markdown
Member

That makes sense if "$" needs replacing with "/", except when I try using a
director, this code is not invoked. It seems that further changes you made
make it irrelevant. This individual code change needed a clear test case to
show the fix working.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

I'll check what excluding this commit will break in our code to create a testcase

Comment thread Source/Modules/java.cxx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your test case might trigger this code, but it has no effect in the subsequent code where the new symname variable is passed on. So either provide a testcase that requires this or it should be removed from the patch.

Copy link
Copy Markdown
Contributor Author

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 I understood you correctly, but there's a case when "symname" is affected and later restored

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But you're correct that removing this code does not affect output

@wsfulton

Copy link
Copy Markdown
Member

Thanks that will really help as I don't have the time to dig out a 3rd party project and learn it to find a suitable testcase.

@wsfulton

Copy link
Copy Markdown
Member

@alexey-pelykh any progress on getting a testcase?

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Sorry, will try to provide those by Monday. Our project entered pre-release stage, too much things to do

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

I've removed that portion of code, indeed it didn't change anything in output even on our codebase

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Something like

struct Outer
{
    %feature("director") Inner
    struct Inner
    {
        virtual ~Inner() {}
        virutal void method() = 0;
    }
};

should trigger the issue solved by e7db081

@wsfulton wsfulton merged commit 837d2bf into swig:master Dec 7, 2014
@wsfulton

wsfulton commented Dec 8, 2014

Copy link
Copy Markdown
Member

Thanks for your changes. I've simplified some of them and made changes to support strongly typed enums in the other languages. I've added runtime tests for numerous languages for the cpp11_strongly_typed_enumerations testcase.

I've also added your director testcase in director_nested_class.i. Although it seems to work in Java, I havn't enabled this test as there are still nested class issues with directors... I don't think this has been tested at all since adding in the nested class support. I suggest raising a separate issue for this.

@alexey-pelykh alexey-pelykh deleted the cpp11_strongly_typed_enums__direct_inject_in_java branch December 10, 2014 10:46
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Thanks for merging changes.

We use nested classes as directors a lot for Java, so far it works. But we've encountered other issues with SWIG in general, which I haven't looked at so far, just used workarounds:

  1. const correctness. In directors, consts are usually misinterpreted. SWIG emits native code to assign values to const arguments of methods. Workaround - don't use "const" arguments in directors.
  2. uint64_t type. SWIG assumes it's "unsigned long long", but on CLang this assumption is incorrect and CLang refuses to compile, asking for explicit cast. Solution - never ever mention uint64_t in SWIG-related classes.
  3. directors + shared_ptr. While I understand why being director and being declared as shared_ptr may not work, I'm very confused why director can not use shared_ptr classes as arguments or return values. SWIG simply does resolve shared_ptr'ed types in directors and they remain SWIG_std...

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.

2 participants