From 01277d700ca44e695d584838dc42c98e035ad514 Mon Sep 17 00:00:00 2001 From: Thomas Reitmayr Date: Tue, 9 Jun 2020 21:58:47 +0200 Subject: [PATCH] Unwrap director classes only when returning a pointer or reference to an object This involves properly counting the number of references and pointers in the return type of a function and only generate unwrapping code if this number is 1. For template instances some post-processing code is added to fix the 'decl' and 'type' attributes of functions if changed in an unfavorable way during template expansion. This commit fixes swig#1811. --- Examples/test-suite/common.mk | 1 + Examples/test-suite/director_unwrap_result.i | 82 ++++++++++++++ Examples/test-suite/javascript/Makefile.in | 1 + .../ruby/director_unwrap_result_runme.rb | 106 ++++++++++++++++++ Source/CParse/templ.c | 91 +++++++++++++++ Source/Modules/python.cxx | 4 +- Source/Modules/ruby.cxx | 4 +- Source/Swig/swig.h | 2 + Source/Swig/typeobj.c | 84 ++++++++++++++ 9 files changed, 369 insertions(+), 6 deletions(-) create mode 100644 Examples/test-suite/director_unwrap_result.i create mode 100644 Examples/test-suite/ruby/director_unwrap_result_runme.rb diff --git a/Examples/test-suite/common.mk b/Examples/test-suite/common.mk index e77f09c86..abc1e9997 100644 --- a/Examples/test-suite/common.mk +++ b/Examples/test-suite/common.mk @@ -213,6 +213,7 @@ CPP_TEST_CASES += \ director_smartptr \ director_thread \ director_unroll \ + director_unwrap_result \ director_using \ director_void \ director_wombat \ diff --git a/Examples/test-suite/director_unwrap_result.i b/Examples/test-suite/director_unwrap_result.i new file mode 100644 index 000000000..bcb7f0fcc --- /dev/null +++ b/Examples/test-suite/director_unwrap_result.i @@ -0,0 +1,82 @@ +%module(directors="1") director_unwrap_result + +%warnfilter(SWIGWARN_TYPEMAP_DIRECTOROUT_PTR) Storage; +%warnfilter(SWIGWARN_TYPEMAP_DIRECTOROUT_PTR) StorageTmpl; + +%feature("director") Element; +%feature("director") Storage; +%feature("director") StorageTmpl; + +%inline %{ + +class Element { + Element* self; + Element** selfptr; + public: + Element() { + self = this; + selfptr = &self; + } + virtual ~Element() {} + Element **getPtrPtr() { + return &self; + } + Element ***getPtrPtrPtr() { + return &selfptr; + } +}; + +class Storage { + public: + virtual ~Storage() {} + virtual Element **getIt() = 0; + Element getElement() { + return **getIt(); + } + Element* const getElementPtr() { + return *getIt(); + } + Element& getElementRef() { + return **getIt(); + } + Element* const *getElementPtrPtr() { + return getIt(); + } + Element *&getElementPtrRef() { + return *getIt(); + } +}; + +template class StorageTmpl { + public: + virtual ~StorageTmpl() {} + virtual T &getIt() = 0; + T getVal() { + return getIt(); + } + T *getPtr() { + return &getIt(); + } + T &getRef() { + return getIt(); + } +}; + +%} + +%template(ElementStorage) StorageTmpl; +%template(ElementPtrStorage) StorageTmpl; +%template(ElementPtrPtrStorage) StorageTmpl; + +%inline %{ + +template T getParam(T t) { + return t; +} + +%} + +%template(getIntParam) getParam; +%template(getIntPtrParam) getParam; +%template(getElementPtrParam) getParam; + diff --git a/Examples/test-suite/javascript/Makefile.in b/Examples/test-suite/javascript/Makefile.in index 8127415f1..bf7f24236 100644 --- a/Examples/test-suite/javascript/Makefile.in +++ b/Examples/test-suite/javascript/Makefile.in @@ -52,6 +52,7 @@ ifeq (node,$(JSENGINE)) apply_signed_char.cpptest: GYP_CFLAGS = \"-Wno-ignored-qualifiers\" constant_pointers.cpptest: GYP_CFLAGS = \"-Wno-ignored-qualifiers\" enum_thorough.cpptest: GYP_CFLAGS = \"-Wno-ignored-qualifiers\" + director_unwrap_result.cpptest: GYP_CFLAGS = \"-Wno-ignored-qualifiers\" setup_node = \ test -d $* || mkdir $* && \ diff --git a/Examples/test-suite/ruby/director_unwrap_result_runme.rb b/Examples/test-suite/ruby/director_unwrap_result_runme.rb new file mode 100644 index 000000000..56970b3e8 --- /dev/null +++ b/Examples/test-suite/ruby/director_unwrap_result_runme.rb @@ -0,0 +1,106 @@ +#!/usr/bin/env ruby +# +# This test checks the proper unwrapping of director objects before returning +# a pointer to the (wrapped) instance. +# Unwrapping must not happen for return-by-value and returning higher +# reference levels (pointer to pointer, reference to pointer, etc.), but this +# is already checked by the C++ compiler. +# + +require 'swig_assert' + +require 'director_unwrap_result' + +############################ +# test with a regular (non-template) class + +class MyElement < Director_unwrap_result::Element +end + +class MyStorage < Director_unwrap_result::Storage + def initialize(e) + super() + @elem = e + end + def getIt + @elem.getPtrPtr + end +end + +e = MyElement.new +s = MyStorage.new(e) + +swig_assert_equal('s.getElement.class', 'Director_unwrap_result::Element', binding) +swig_assert('s.getElement != e', binding) + +# this shows that the director class was unwrapped: +swig_assert_equal('s.getElementPtr.class', 'MyElement', binding) +swig_assert_equal('s.getElementPtr', 'e', binding) + +# this shows that the director class was unwrapped: +swig_assert_equal('s.getElementRef.class', 'MyElement', binding) +swig_assert_equal('s.getElementRef', 'e', binding) + +swig_assert_equal('s.getElementPtrPtr.class', 'SWIG::TYPE_p_p_Element', binding) +swig_assert_equal('s.getElementPtrPtr.class', 'SWIG::TYPE_p_p_Element', binding) + +swig_assert_equal('s.getElementPtrRef.class', 'SWIG::TYPE_p_p_Element', binding) +swig_assert_equal('s.getElementPtrRef.class', 'SWIG::TYPE_p_p_Element', binding) + +############################ +# test with a template class + +class MyElementStorage < Director_unwrap_result::ElementStorage + def initialize(e) + super() + @elem = e + end + def getIt + @elem + end +end + +class MyElementPtrStorage < Director_unwrap_result::ElementPtrStorage + def initialize(e) + super() + @elem = e + end + def getIt + @elem.getPtrPtr + end +end + +class MyElementPtrPtrStorage < Director_unwrap_result::ElementPtrPtrStorage + def initialize(e) + super() + @elem = e + end + def getIt + @elem.getPtrPtrPtr + end +end + +e = MyElement.new + +s = MyElementStorage.new(e) +swig_assert_equal('s.getVal.class', 'Director_unwrap_result::Element', binding) +swig_assert('s.getVal != e', binding) +# this shows that the director class was unwrapped: +swig_assert_equal('s.getPtr.class', 'MyElement', binding) +swig_assert_equal('s.getPtr', 'e', binding) +# this shows that the director class was unwrapped: +swig_assert_equal('s.getRef.class', 'MyElement', binding) +swig_assert_equal('s.getRef', 'e', binding) + +s = MyElementPtrStorage.new(e) +# this shows that the director class was unwrapped: +swig_assert_equal('s.getVal.class', 'MyElement', binding) +swig_assert_equal('s.getVal', 'e', binding) +swig_assert_equal('s.getPtr.class', 'SWIG::TYPE_p_p_Element', binding) +swig_assert_equal('s.getRef.class', 'SWIG::TYPE_p_p_Element', binding) + +s = MyElementPtrPtrStorage.new(e) +swig_assert_equal('s.getVal.class', 'SWIG::TYPE_p_p_Element', binding) +swig_assert_equal('s.getPtr.class', 'SWIG::TYPE_p_p_p_Element', binding) +swig_assert_equal('s.getRef.class', 'SWIG::TYPE_p_p_p_Element', binding) + diff --git a/Source/CParse/templ.c b/Source/CParse/templ.c index 22d49fac5..5f43bad82 100644 --- a/Source/CParse/templ.c +++ b/Source/CParse/templ.c @@ -234,6 +234,95 @@ static void cparse_template_expand(Node *templnode, Node *n, String *tname, Stri } } +/* ----------------------------------------------------------------------------- + * cparse_fix_function_decl() + * + * Move the prefix of the "type" attribute (excluding any trailing qualifier) + * to the end of the "decl" attribute. + * Examples: + * decl="f().", type="p.q(const).char" => decl="f().p.", type="q(const).char" + * decl="f().p.", type="p.SomeClass" => decl="f().p.p.", type="SomeClass" + * decl="f().", type="r.q(const).p.int" => decl="f().r.q(const).p.", type="int" + * ----------------------------------------------------------------------------- */ + +static void cparse_fix_function_decl(String *name, SwigType *decl, SwigType *type) { + String *prefix; + int prefixLen; + SwigType* last; + + /* The type's prefix is what potentially has to be moved to the end of 'decl' */ + prefix = SwigType_prefix(type); + + /* First some parts (qualifier and array) have to be removed from prefix + in order to remain in the 'type' attribute. */ + last = SwigType_last(prefix); + while (last) { + if (SwigType_isqualifier(last) || SwigType_isarray(last)) { + /* Keep this part in the 'type' */ + Delslice(prefix, Len(prefix) - Len(last), DOH_END); + Delete(last); + last = SwigType_last(prefix); + } else { + /* Done with processing prefix */ + Delete(last); + last = 0; + } + } + + /* Transfer prefix from the 'type' to the 'decl' attribute */ + prefixLen = Len(prefix); + if (prefixLen > 0) { + Append(decl, prefix); + Delslice(type, 0, prefixLen); + if (template_debug) { + Printf(stdout, " change function '%s' to type='%s', decl='%s'\n", + name, type, decl); + } + } + + Delete(prefix); +} + +/* ----------------------------------------------------------------------------- + * cparse_postprocess_expanded_template() + * + * This function postprocesses the given node after template expansion. + * Currently the only task to perform is fixing function decl and type attributes. + * ----------------------------------------------------------------------------- */ + +static void cparse_postprocess_expanded_template(Node *n) { + String *nodeType; + if (!n) + return; + nodeType = nodeType(n); + if (Getattr(n, "error")) + return; + + if (Equal(nodeType, "cdecl")) { + /* A simple C declaration */ + SwigType *d = Getattr(n, "decl"); + if (d && SwigType_isfunction(d)) { + /* A function node */ + SwigType *t = Getattr(n, "type"); + if (t) { + String *name = Getattr(n, "name"); + cparse_fix_function_decl(name, d, t); + } + } + } else { + /* Look for any children */ + Node *cn = firstChild(n); + while (cn) { + cparse_postprocess_expanded_template(cn); + cn = nextSibling(cn); + } + } +} + +/* ----------------------------------------------------------------------------- + * partial_arg() + * ----------------------------------------------------------------------------- */ + static String *partial_arg(String *s, String *p) { char *c; @@ -414,6 +503,8 @@ int Swig_cparse_template_expand(Node *n, String *rname, ParmList *tparms, Symtab } } + cparse_postprocess_expanded_template(n); + /* Patch bases */ { List *bases = Getattr(n, "baselist"); diff --git a/Source/Modules/python.cxx b/Source/Modules/python.cxx index c8c45df35..336f6c219 100644 --- a/Source/Modules/python.cxx +++ b/Source/Modules/python.cxx @@ -3118,9 +3118,7 @@ public: #if 1 int unwrap = 0; String *decl = Getattr(n, "decl"); - int is_pointer = SwigType_ispointer_return(decl); - int is_reference = SwigType_isreference_return(decl); - if (is_pointer || is_reference) { + if (SwigType_refptr_count_return(decl) == 1) { String *type = Getattr(n, "type"); //Node *classNode = Swig_methodclass(n); //Node *module = Getattr(classNode, "module"); diff --git a/Source/Modules/ruby.cxx b/Source/Modules/ruby.cxx index 48b0efab3..64bc4df89 100644 --- a/Source/Modules/ruby.cxx +++ b/Source/Modules/ruby.cxx @@ -1893,9 +1893,7 @@ public: */ bool unwrap = false; String *decl = Getattr(n, "decl"); - int is_pointer = SwigType_ispointer_return(decl); - int is_reference = SwigType_isreference_return(decl); - if (is_pointer || is_reference) { + if (SwigType_refptr_count_return(decl) == 1) { String *type = Getattr(n, "type"); Node *parent = Swig_methodclass(n); Node *modname = Getattr(parent, "module"); diff --git a/Source/Swig/swig.h b/Source/Swig/swig.h index 76691269e..2bf3e08b0 100644 --- a/Source/Swig/swig.h +++ b/Source/Swig/swig.h @@ -141,6 +141,8 @@ extern "C" { extern List *SwigType_split(const SwigType *t); extern String *SwigType_pop(SwigType *t); extern void SwigType_push(SwigType *t, String *s); + extern SwigType *SwigType_last(SwigType *t); + extern int SwigType_refptr_count_return(const SwigType *t); extern List *SwigType_parmlist(const SwigType *p); extern String *SwigType_parm(const SwigType *p); extern String *SwigType_str(const SwigType *s, const_String_or_char_ptr id); diff --git a/Source/Swig/typeobj.c b/Source/Swig/typeobj.c index 69fb6662b..8e64bad92 100644 --- a/Source/Swig/typeobj.c +++ b/Source/Swig/typeobj.c @@ -208,6 +208,90 @@ SwigType *SwigType_pop(SwigType *t) { return result; } +/* ----------------------------------------------------------------------------- + * SwigType_last() + * + * Return the last element of the given (partial) type. + * For example: + * t: q(const).p. + * result: p. + * ----------------------------------------------------------------------------- */ + +SwigType *SwigType_last(SwigType *t) { + SwigType *result; + char *c; + char *last; + int sz = 0; + + if (!t) + return 0; + + /* Find the last element */ + c = Char(t); + last = 0; + while (*c) { + last = c; + sz = element_size(c); + c = c + sz; + if (*c == '.') { + c++; + sz++; + } + } + + /* Extract the last element */ + if (last) { + result = NewStringWithSize(last, sz); + } else { + result = 0; + } + + return result; +} + +/* ----------------------------------------------------------------------------- + * SwigType_refptr_count_return() + * + * Returns the number of pointer and reference indirections found in the given + * type. For functions this concerns the return type. + * For example: + * r.p. => 2 + * q(const).p. => 1 + * r.f(int).p.p. => 2 + * f().p.q(const).p. => 2 + * ----------------------------------------------------------------------------- */ + +int SwigType_refptr_count_return(const SwigType *t) { + char *c; + char *last; + int sz; + int result = 0; + + if (!t) + return 0; + + c = Char(t); + last = c; + while (*c) { + last = c; + sz = element_size(c); + c = c + sz; + if (*(c-1) == '.') { + if (((last[0] == 'p') || (last[0] == 'r')) && (last[1] == '.')) { + result++; + } else if (strncmp(last, "f(", 2) == 0) { + /* restart counter if this is a function type */ + result = 0; + } + } + if (*c == '.') { + c++; + } + } + + return result; +} + /* ----------------------------------------------------------------------------- * SwigType_parm() *