From eb88070fc2a15f91a96973cec59cba8525cbf888 Mon Sep 17 00:00:00 2001 From: cfis Date: Tue, 13 Jan 2026 20:50:48 -0800 Subject: [PATCH 1/8] Add iterator test for using lambda. --- test/test_Iterator.cpp | 133 +++++++++++++++++++++++++++++------------ 1 file changed, 95 insertions(+), 38 deletions(-) diff --git a/test/test_Iterator.cpp b/test/test_Iterator.cpp index 5a6fe9b9..193d9bb1 100644 --- a/test/test_Iterator.cpp +++ b/test/test_Iterator.cpp @@ -6,16 +6,6 @@ using namespace Rice; TESTSUITE(Iterator); -SETUP(Iterator) -{ - embed_ruby(); -} - -TEARDOWN(Iterator) -{ - rb_gc_start(); -} - namespace { class Container @@ -36,24 +26,6 @@ namespace int* array_; size_t length_; }; -} // namespace - -TESTCASE(define_array_iterator) -{ - define_class("Container") - .define_constructor(Constructor()) - .define_iterator(&Container::beginFoo, &Container::endBar); - - int array[] = { 1, 2, 3 }; - Container* container = new Container(array, 3); - - Object wrapped_container = Data_Object(container); - - Array a = wrapped_container.instance_eval("a = []; each() { |x| a << x }; a"); - ASSERT_EQUAL(3, a.size()); - ASSERT_EQUAL(Object(detail::to_ruby(1)), a[0]); - ASSERT_EQUAL(Object(detail::to_ruby(2)), a[1]); - ASSERT_EQUAL(Object(detail::to_ruby(3)), a[2]); } namespace @@ -139,7 +111,41 @@ namespace }; } -TESTCASE(iterator_value) + +SETUP(Iterator) +{ + embed_ruby(); +} + +TEARDOWN(Iterator) +{ + Data_Type::unbind(); + Data_Type::unbind(); + Rice::detail::Registries::instance.types.remove(); + Rice::detail::Registries::instance.types.remove(); + + rb_gc_start(); +} + +TESTCASE(ArrayIterator) +{ + define_class("Container") + .define_constructor(Constructor()) + .define_iterator(&Container::beginFoo, &Container::endBar); + + int array[] = { 1, 2, 3 }; + Container* container = new Container(array, 3); + + Object wrapped_container = Data_Object(container); + + Array a = wrapped_container.instance_eval("a = []; each() { |x| a << x }; a"); + ASSERT_EQUAL(3, a.size()); + ASSERT_EQUAL(Object(detail::to_ruby(1)), a[0]); + ASSERT_EQUAL(Object(detail::to_ruby(2)), a[1]); + ASSERT_EQUAL(Object(detail::to_ruby(3)), a[2]); +} + +TESTCASE(Standard) { define_class("Data") .define_constructor(Constructor()); @@ -164,7 +170,57 @@ TESTCASE(iterator_value) ASSERT_EQUAL(3u, wrappedData->index); } -TESTCASE(const_iterator_value) +TESTCASE(Lambda) +{ + define_class("Data") + .define_constructor(Constructor()); + + define_class("ContainerValues") + .define_constructor(Constructor()) + .include_module(rb_mEnumerable) + .define_method("each", [](VALUE self, VALUE proc)->VALUE + { + ContainerValues* container = detail::From_Ruby().convert(self); + + // The iterator returns references - we do NOT want to create a copy + detail::To_Ruby toRuby; + + auto it = container->begin(); + auto end = container->end(); + + for (; it != end; ++it) + { + detail::protect(rb_yield, toRuby.convert(*it)); + } + + return self; + }, Arg("proc").setValue()); + + ContainerValues* container = new ContainerValues(); + Data_Object wrapper(container); + + Module m = define_module("TestIterator"); + std::string code = R"(result = [] + container = ContainerValues.new + container.each do |data| + result << data + end + result)"; + + Array result = m.module_eval(code); + ASSERT_EQUAL(3, result.size()); + + Data_Object wrappedData(result[0]); + ASSERT_EQUAL(1u, wrappedData->index); + + wrappedData = (Data_Object)result[1]; + ASSERT_EQUAL(2u, wrappedData->index); + + wrappedData = (Data_Object)result[2]; + ASSERT_EQUAL(3u, wrappedData->index); +} + +TESTCASE(ConstValue) { define_class("Data") .define_constructor(Constructor()); @@ -194,7 +250,7 @@ TESTCASE(const_iterator_value) ASSERT_EQUAL(3u, wrappedData->index); } -TESTCASE(iterator_pointer) +TESTCASE(Pointer) { define_class("Data") .define_constructor(Constructor()); @@ -227,7 +283,7 @@ TESTCASE(iterator_pointer) ASSERT_EQUAL(3u, wrappedData->index); } -TESTCASE(two_iterator_pointer) +TESTCASE(TwoIteratorPointers) { define_class("Data") .define_constructor(Constructor()); @@ -275,7 +331,7 @@ TESTCASE(two_iterator_pointer) ASSERT_EQUAL(1u, wrappedData->index); } -TESTCASE(map) +TESTCASE(Map) { define_class("Data") .define_constructor(Constructor()) @@ -305,10 +361,11 @@ TESTCASE(map) ASSERT_EQUAL(6, detail::From_Ruby().convert(element)); } -TESTCASE(to_enum) +TESTCASE(Enum) { define_class("Data") - .define_constructor(Constructor()); + .define_constructor(Constructor()) + .define_attr("index", &Data::index, Rice::AttrAccess::Read); define_class("ContainerPointers") .define_constructor(Constructor()) @@ -335,7 +392,7 @@ TESTCASE(to_enum) ASSERT_EQUAL(6, detail::From_Ruby().convert(element)); } -TESTCASE(to_a) +TESTCASE(ToArray) { define_class("Data") .define_constructor(Constructor()) @@ -372,9 +429,9 @@ TESTCASE(IterateNoCopy) define_class("Data") .define_constructor(Constructor()); - define_class("ContainerValues") + define_class("ContainerValues") .define_constructor(Constructor()) - .define_iterator(&ContainerPointers::begin, &ContainerPointers::end); + .define_iterator(&ContainerValues::begin, &ContainerValues::end); Module m = define_module("Testing"); From e3b40d3a23240a104400a064fc9ca8a0549a6e31 Mon Sep 17 00:00:00 2001 From: cfis Date: Tue, 13 Jan 2026 20:50:58 -0800 Subject: [PATCH 2/8] Comment back in tests. --- test/test_Data_Type.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_Data_Type.cpp b/test/test_Data_Type.cpp index 04944e65..59e9dbb0 100644 --- a/test/test_Data_Type.cpp +++ b/test/test_Data_Type.cpp @@ -20,7 +20,7 @@ TEARDOWN(Data_Type) rb_gc_start(); } -/*namespace +namespace { class MyClass { @@ -499,7 +499,7 @@ TESTCASE(null_ptrs) result = o.call("set", nullptr); ASSERT_EQUAL(Qnil, result.value()); -}*/ +} namespace { From 807498bc8a0acd11607e5cf601219de51c53bf88 Mon Sep 17 00:00:00 2001 From: cfis Date: Tue, 13 Jan 2026 22:22:11 -0800 Subject: [PATCH 3/8] Test passing nil to custom iterator. --- test/test_Iterator.cpp | 47 ++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/test/test_Iterator.cpp b/test/test_Iterator.cpp index 193d9bb1..49e2d19e 100644 --- a/test/test_Iterator.cpp +++ b/test/test_Iterator.cpp @@ -180,21 +180,30 @@ TESTCASE(Lambda) .include_module(rb_mEnumerable) .define_method("each", [](VALUE self, VALUE proc)->VALUE { - ContainerValues* container = detail::From_Ruby().convert(self); + if (!detail::protect(rb_block_given_p)) + { + static Identifier identifier("each"); + VALUE enumerator = detail::protect(rb_enumeratorize_with_size, self, identifier.to_sym(), 0, nullptr, nullptr); + return enumerator; + } + else + { + ContainerValues* container = detail::From_Ruby().convert(self); - // The iterator returns references - we do NOT want to create a copy - detail::To_Ruby toRuby; + // The iterator returns references - we do NOT want to create a copy + detail::To_Ruby toRuby; - auto it = container->begin(); - auto end = container->end(); + auto it = container->begin(); + auto end = container->end(); - for (; it != end; ++it) - { - detail::protect(rb_yield, toRuby.convert(*it)); - } + for (; it != end; ++it) + { + detail::protect(rb_yield, toRuby.convert(*it)); + } - return self; - }, Arg("proc").setValue()); + return self; + } + }, Arg("proc").setValue() = Qnil, Return().setValue()); ContainerValues* container = new ContainerValues(); Data_Object wrapper(container); @@ -218,6 +227,22 @@ TESTCASE(Lambda) wrappedData = (Data_Object)result[2]; ASSERT_EQUAL(3u, wrappedData->index); + + code = R"(container = ContainerValues.new + enumerator = container.each + enumerator.to_a)"; + + result = m.module_eval(code); + ASSERT_EQUAL(3, result.size()); + + wrappedData = (Data_Object)result[0]; + ASSERT_EQUAL(1u, wrappedData->index); + + wrappedData = (Data_Object)result[1]; + ASSERT_EQUAL(2u, wrappedData->index); + + wrappedData = (Data_Object)result[2]; + ASSERT_EQUAL(3u, wrappedData->index); } TESTCASE(ConstValue) From 9a0f4343d6d47669cff9fa67344f85c240d37ca5 Mon Sep 17 00:00:00 2001 From: cfis Date: Sat, 17 Jan 2026 00:06:54 -0800 Subject: [PATCH 4/8] Support default values for incomplete types. --- CHANGELOG.md | 10 ++++ rice/Arg.hpp | 1 + rice/detail/Parameter.ipp | 37 ++++++++++---- test/CMakeLists.txt | 57 +-------------------- test/test_Buffer.cpp | 57 +++++++++++++++++++++ test/test_Incomplete.cpp | 101 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 197 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1962ef75..83ebe40b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,16 @@ Enhancements: Internal: * Refactor type handling by merging `TypeMapper` into `TypeDetail` and simplifying class hierarchy +Incomptatible Changes: +* Rice converts Ruby blocks to procs. Thus if you have a method that expects a block, you must add + a VALUE parameter and tell Rice it is a value. For example: + + define_method("my_method", [](VALUE self, VALUE proc) + { + }, Arg("proc").setValue()) + + Think of this as similar to how you would capture a block in Ruby using the &block syntax. + ## 4.9.1 (2026-01-04) This release focuses on improving memory management for STL containers and attribute setters. diff --git a/rice/Arg.hpp b/rice/Arg.hpp index 16dff4fb..4df6f550 100644 --- a/rice/Arg.hpp +++ b/rice/Arg.hpp @@ -97,6 +97,7 @@ namespace Rice { public: ArgBuffer(std::string name); + using Arg::operator=; // Inherit the templated operator= }; } // Rice diff --git a/rice/detail/Parameter.ipp b/rice/detail/Parameter.ipp index 7e6e08e2..63728db3 100644 --- a/rice/detail/Parameter.ipp +++ b/rice/detail/Parameter.ipp @@ -127,9 +127,27 @@ namespace Rice::detail template inline VALUE Parameter::defaultValueRuby() { + using To_Ruby_T = remove_cv_recursive_t; + + if (!this->arg()->hasDefaultValue()) + { + throw std::runtime_error("No default value set for " + this->arg()->name); + } + if constexpr (!is_complete_v>) { - // Incomplete types can't have default values (std::any requires complete types) + // Only pointers to incomplete types can have + // default values (e.g., void* or Impl*), since the pointer itself is complete. + // References and values of incomplete types cannot be stored in std::any. + if constexpr (std::is_pointer_v>) + { + T defaultValue = this->arg()->template defaultValue(); + return this->toRuby_.convert((To_Ruby_T)defaultValue); + } + else + { + throw std::runtime_error("Default value not allowed for incomple type. Parameter " + this->arg()->name); + } } else if constexpr (std::is_constructible_v, std::remove_cv_t>&>) { @@ -139,21 +157,20 @@ namespace Rice::detail { if constexpr (std::is_copy_constructible_v::value_type>) { - if (this->arg()->hasDefaultValue()) - { - T defaultValue = this->arg()->template defaultValue(); - return this->toRuby_.convert(defaultValue); - } + T defaultValue = this->arg()->template defaultValue(); + return this->toRuby_.convert(defaultValue); } } - else if (this->arg()->hasDefaultValue()) + else { T defaultValue = this->arg()->template defaultValue(); - return this->toRuby_.convert((remove_cv_recursive_t)defaultValue); + return this->toRuby_.convert((To_Ruby_T)defaultValue); } } - - throw std::runtime_error("No default value set or allowed for parameter " + this->arg()->name); + else + { + throw std::runtime_error("Default value not allowed for parameter " + this->arg()->name); + } } template diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e48813c9..a2855714 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -41,61 +41,6 @@ target_include_directories(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/. target_sources(${PROJECT_NAME} PRIVATE "unittest.cpp" "embed_ruby.cpp" - "test_Address_Registration_Guard.cpp" - "test_Array.cpp" - "test_Attribute.cpp" "test_Buffer.cpp" - "test_Builtin_Object.cpp" - "test_Callback.cpp" - "test_Class.cpp" - "test_Constructor.cpp" - "test_Data_Object.cpp" - "test_Data_Type.cpp" - "test_Director.cpp" - "test_Enum.cpp" - "test_Exception.cpp" - "test_File.cpp" - "test_From_Ruby.cpp" - "test_GVL.cpp" - "test_Hash.cpp" - "test_global_functions.cpp" - "test_Identifier.cpp" "test_Incomplete.cpp" - "test_Inheritance.cpp" - "test_Iterator.cpp" - "test_Jump_Exception.cpp" - "test_Keep_Alive.cpp" - "test_Keep_Alive_No_Wrapper.cpp" - "test_Memory_Management.cpp" - "test_Module.cpp" - "test_Native_Registry.cpp" - "test_Object.cpp" - "test_Overloads.cpp" - "test_Ownership.cpp" - "test_Proc.cpp" - "test_Reference.cpp" - "test_Self.cpp" - "test_String.cpp" - "test_Struct.cpp" - "test_Symbol.cpp" - "test_Template.cpp" - "test_To_Ruby.cpp" - "test_Tracking.cpp" - "test_Type.cpp" - "test_Stl_Exception.cpp" - "test_Stl_Map.cpp" - "test_Stl_Multimap.cpp" - "test_Stl_Optional.cpp" - "test_Stl_OStream.cpp" - "test_Stl_Pair.cpp" - "test_Stl_Reference_Wrapper.cpp" - "test_Stl_Set.cpp" - "test_Stl_SharedPtr.cpp" - "test_Stl_String.cpp" - "test_Stl_String_View.cpp" - "test_Stl_Tuple.cpp" - "test_Stl_Type_Info.cpp" - "test_Stl_UniquePtr.cpp" - "test_Stl_Unordered_Map.cpp" - "test_Stl_Variant.cpp" - "test_Stl_Vector.cpp") +) diff --git a/test/test_Buffer.cpp b/test/test_Buffer.cpp index 49fc09ac..48541e5b 100644 --- a/test/test_Buffer.cpp +++ b/test/test_Buffer.cpp @@ -623,4 +623,61 @@ TESTCASE(void_pointer_function) // This should compile and run without errors void** result = getVoidPtrs(); ASSERT_EQUAL(&value1, result[0]); +} + +namespace +{ + static int defaultIntBuffer[] = { 10, 20, 30 }; + static int secretNumber = 42; + + int processIntBuffer(int* buffer, size_t size) + { + int sum = 0; + for (size_t i = 0; i < size; i++) + { + sum += buffer[i]; + } + return sum; + } + + int processIntBufferWithDefault(int* buffer = defaultIntBuffer, size_t size = 3) + { + return processIntBuffer(buffer, size); + } + + int processVoidPtrWithDefault(void* ptr = &secretNumber) + { + return *static_cast(ptr); + } +} + +TESTCASE(arg_buffer_int_default) +{ + define_buffer(); + Module m = define_module("Testing"); + m.define_module_function("process_int_buffer", &processIntBufferWithDefault, + ArgBuffer("buffer") = static_cast(defaultIntBuffer), Arg("size") = static_cast(3)); + + // Call with no arguments - should use defaults (10 + 20 + 30 = 60) + std::string code = R"(process_int_buffer())"; + Object result = m.module_eval(code); + ASSERT_EQUAL(60, detail::From_Ruby().convert(result)); + + // Call with buffer + code = R"(buffer = Rice::Buffer≺int≻.new([1, 2, 3]) + process_int_buffer(buffer.data, buffer.size))"; + result = m.module_eval(code); + ASSERT_EQUAL(6, detail::From_Ruby().convert(result)); +} + +TESTCASE(arg_buffer_void_default) +{ + Module m = define_module("Testing"); + m.define_module_function("process_void_ptr", &processVoidPtrWithDefault, + ArgBuffer("ptr") = static_cast(&secretNumber)); + + // Call with no arguments - should use default (42) + std::string code = R"(process_void_ptr())"; + Object result = m.module_eval(code); + ASSERT_EQUAL(42, detail::From_Ruby().convert(result)); } \ No newline at end of file diff --git a/test/test_Incomplete.cpp b/test/test_Incomplete.cpp index 790071c6..00d5da64 100644 --- a/test/test_Incomplete.cpp +++ b/test/test_Incomplete.cpp @@ -489,4 +489,105 @@ TESTCASE(IncompleteReferenceModify) Object result = m.module_eval(code); ASSERT_EQUAL(456, detail::From_Ruby().convert(result.value())); +} + +// ======================================================================= +// Default Values with Incomplete Types +// +// Test that incomplete types can have default values (pointers only - +// references and values can't have meaningful defaults for incomplete types) +// ======================================================================= + +namespace +{ + // Forward declaration only - DefaultImpl is never defined + class DefaultImpl; + + struct DefaultImplReal + { + int value = 999; + }; + + // Static instance for non-null default + static DefaultImplReal defaultImplInstance{ 777 }; + static DefaultImpl* defaultImplPtr = reinterpret_cast(&defaultImplInstance); + + // Function with pointer to incomplete type and non-null default value + int processDefaultImpl(DefaultImpl* impl = defaultImplPtr) + { + return reinterpret_cast(impl)->value; + } + + // Function with pointer to incomplete type, default value, and other params + int processDefaultImplWithSize(DefaultImpl* impl = defaultImplPtr, int multiplier = 1) + { + return reinterpret_cast(impl)->value * multiplier; + } + + DefaultImpl* createDefaultImpl() + { + return reinterpret_cast(new DefaultImplReal()); + } + + void destroyDefaultImpl(DefaultImpl* impl) + { + delete reinterpret_cast(impl); + } +} + +TESTCASE(IncompletePointerDefaultValue) +{ + // Register the incomplete type + define_class("DefaultImpl"); + + // Test function with pointer to incomplete type and non-null default + define_global_function("process_default_impl", &processDefaultImpl, + Arg("impl") = defaultImplPtr); + + define_global_function("create_default_impl", &createDefaultImpl); + define_global_function("destroy_default_impl", &destroyDefaultImpl); + + Module m = define_module("TestingModule"); + + // Call with no arguments - should use default (777) + std::string code = R"(process_default_impl())"; + Object result = m.module_eval(code); + ASSERT_EQUAL(777, detail::From_Ruby().convert(result.value())); + + // Call with actual impl (999) + code = R"(impl = create_default_impl() + result = process_default_impl(impl) + destroy_default_impl(impl) + result)"; + result = m.module_eval(code); + ASSERT_EQUAL(999, detail::From_Ruby().convert(result.value())); +} + +TESTCASE(IncompletePointerDefaultValueWithOtherParams) +{ + // Register the incomplete type + define_class("DefaultImpl2"); + + // Test function with pointer to incomplete type, default value, and other params + define_global_function("process_default_impl_with_size", &processDefaultImplWithSize, + Arg("impl") = defaultImplPtr, + Arg("multiplier") = 1); + + define_global_function("create_default_impl", &createDefaultImpl); + define_global_function("destroy_default_impl", &destroyDefaultImpl); + + Module m = define_module("TestingModule"); + + // Call with no arguments - should use defaults (777 * 1) + std::string code = R"(process_default_impl_with_size())"; + Object result = m.module_eval(code); + ASSERT_EQUAL(777, detail::From_Ruby().convert(result.value())); + + // Call with impl and multiplier (999 * 2) + code = R"(impl = create_default_impl() + result = process_default_impl_with_size(impl, 2) + destroy_default_impl(impl) + result)"; + result = m.module_eval(code); + ASSERT_EQUAL(1998, detail::From_Ruby().convert(result.value())); } \ No newline at end of file From 4f2beb11c4c9a407d92324271c7d8b7709482d7c Mon Sep 17 00:00:00 2001 From: cfis Date: Sat, 17 Jan 2026 17:41:41 -0800 Subject: [PATCH 5/8] Fix support for ** to incomplee types. --- rice/Buffer.ipp | 23 +++--- rice/Data_Object.ipp | 16 +---- rice/detail/Wrapper.ipp | 9 ++- test/CMakeLists.txt | 57 ++++++++++++++- test/test_Incomplete.cpp | 146 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+), 27 deletions(-) diff --git a/rice/Buffer.ipp b/rice/Buffer.ipp index e2a6135b..f66dcaa9 100644 --- a/rice/Buffer.ipp +++ b/rice/Buffer.ipp @@ -944,19 +944,22 @@ namespace Rice define_method("data", &Buffer_T::ptr, ReturnBuffer()). define_method("release", &Buffer_T::release, ReturnBuffer()); - if constexpr (!std::is_pointer_v && !std::is_void_v && !std::is_const_v && std::is_copy_assignable_v) + if constexpr (detail::is_complete_v>) { - klass.define_method("[]=", [](Buffer_T& self, size_t index, T& value) -> void + if constexpr (!std::is_pointer_v && !std::is_void_v && !std::is_const_v && std::is_copy_assignable_v) { - self[index] = value; - }); - } - else if constexpr (std::is_pointer_v && !std::is_const_v> && std::is_copy_assignable_v>) - { - klass.define_method("[]=", [](Buffer_T& self, size_t index, T value) -> void + klass.define_method("[]=", [](Buffer_T& self, size_t index, T& value) -> void + { + self[index] = value; + }); + } + else if constexpr (std::is_pointer_v && !std::is_const_v> && std::is_copy_assignable_v>) { - *self[index] = *value; - }); + klass.define_method("[]=", [](Buffer_T& self, size_t index, T value) -> void + { + *self[index] = *value; + }); + } } return klass; diff --git a/rice/Data_Object.ipp b/rice/Data_Object.ipp index 0db3e39b..0fab290e 100644 --- a/rice/Data_Object.ipp +++ b/rice/Data_Object.ipp @@ -634,15 +634,13 @@ namespace Rice::detail double is_convertible(VALUE value) { - bool isBuffer = dynamic_cast(this->arg_) ? true : false; - switch (rb_type(value)) { case RUBY_T_NIL: return Convertible::Exact; break; case RUBY_T_DATA: - if (Data_Type::is_descendant(value) && isBuffer) + if (Data_Type::is_descendant(value)) { return Convertible::Exact; } @@ -655,7 +653,6 @@ namespace Rice::detail T** convert(VALUE value) { bool isOwner = this->arg_ && this->arg_->isOwner(); - bool isBuffer = dynamic_cast(this->arg_) ? true : false; switch (rb_type(value)) { @@ -666,7 +663,7 @@ namespace Rice::detail } case RUBY_T_DATA: { - if (Data_Type::is_descendant(value) && isBuffer) + if (Data_Type::is_descendant(value)) { T** result = detail::unwrap(value, Data_Type::ruby_data_type(), isOwner); return result; @@ -675,14 +672,7 @@ namespace Rice::detail } default: { - if (isBuffer) - { - throw create_type_exception(value); - } - else - { - throw create_type_exception(value); - } + throw create_type_exception(value); } } } diff --git a/rice/detail/Wrapper.ipp b/rice/detail/Wrapper.ipp index cfd5e4a1..e3feb4de 100644 --- a/rice/detail/Wrapper.ipp +++ b/rice/detail/Wrapper.ipp @@ -155,11 +155,14 @@ namespace Rice::detail { Registries::instance.instances.remove(this->get(this->rb_data_type_)); - if constexpr (std::is_destructible_v) + if constexpr (is_complete_v) { - if (this->isOwner_) + if constexpr (std::is_destructible_v) { - delete this->data_; + if (this->isOwner_) + { + delete this->data_; + } } } } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a2855714..e48813c9 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -41,6 +41,61 @@ target_include_directories(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/. target_sources(${PROJECT_NAME} PRIVATE "unittest.cpp" "embed_ruby.cpp" + "test_Address_Registration_Guard.cpp" + "test_Array.cpp" + "test_Attribute.cpp" "test_Buffer.cpp" + "test_Builtin_Object.cpp" + "test_Callback.cpp" + "test_Class.cpp" + "test_Constructor.cpp" + "test_Data_Object.cpp" + "test_Data_Type.cpp" + "test_Director.cpp" + "test_Enum.cpp" + "test_Exception.cpp" + "test_File.cpp" + "test_From_Ruby.cpp" + "test_GVL.cpp" + "test_Hash.cpp" + "test_global_functions.cpp" + "test_Identifier.cpp" "test_Incomplete.cpp" -) + "test_Inheritance.cpp" + "test_Iterator.cpp" + "test_Jump_Exception.cpp" + "test_Keep_Alive.cpp" + "test_Keep_Alive_No_Wrapper.cpp" + "test_Memory_Management.cpp" + "test_Module.cpp" + "test_Native_Registry.cpp" + "test_Object.cpp" + "test_Overloads.cpp" + "test_Ownership.cpp" + "test_Proc.cpp" + "test_Reference.cpp" + "test_Self.cpp" + "test_String.cpp" + "test_Struct.cpp" + "test_Symbol.cpp" + "test_Template.cpp" + "test_To_Ruby.cpp" + "test_Tracking.cpp" + "test_Type.cpp" + "test_Stl_Exception.cpp" + "test_Stl_Map.cpp" + "test_Stl_Multimap.cpp" + "test_Stl_Optional.cpp" + "test_Stl_OStream.cpp" + "test_Stl_Pair.cpp" + "test_Stl_Reference_Wrapper.cpp" + "test_Stl_Set.cpp" + "test_Stl_SharedPtr.cpp" + "test_Stl_String.cpp" + "test_Stl_String_View.cpp" + "test_Stl_Tuple.cpp" + "test_Stl_Type_Info.cpp" + "test_Stl_UniquePtr.cpp" + "test_Stl_Unordered_Map.cpp" + "test_Stl_Variant.cpp" + "test_Stl_Vector.cpp") diff --git a/test/test_Incomplete.cpp b/test/test_Incomplete.cpp index 00d5da64..fd60e72f 100644 --- a/test/test_Incomplete.cpp +++ b/test/test_Incomplete.cpp @@ -590,4 +590,150 @@ TESTCASE(IncompletePointerDefaultValueWithOtherParams) result)"; result = m.module_eval(code); ASSERT_EQUAL(1998, detail::From_Ruby().convert(result.value())); +} + +namespace +{ + // Forward declaration only - ExtraData is never defined + struct ExtraData; + + struct ExtraDataReal + { + int value = 42; + }; + + // Similar to OpenCV's cv::utils::trace::details::TraceArg + struct TraceArg + { + ExtraData** ppExtra; + const char* name; + int flags; + + TraceArg() : ppExtra(nullptr), name("default"), flags(0) + { + } + + TraceArg(const char* n, int f) : ppExtra(nullptr), name(n), flags(f) + { + } + }; + + // Helper functions for double pointer operations + ExtraData** createExtraDataPtr() + { + ExtraData** pp = new ExtraData*; + *pp = reinterpret_cast(new ExtraDataReal()); + return pp; + } + + void destroyExtraDataPtr(ExtraData** pp) + { + if (pp) + { + delete reinterpret_cast(*pp); + delete pp; + } + } + + int getExtraDataValue(ExtraData** pp) + { + if (pp && *pp) + { + return reinterpret_cast(*pp)->value; + } + return -1; + } + + void setExtraDataValue(ExtraData** pp, int value) + { + if (pp && *pp) + { + reinterpret_cast(*pp)->value = value; + } + } +} + +TESTCASE(DoublePointerIncomplete) +{ + // Register the incomplete type + define_class("ExtraData"); + + // Test struct with double pointer to incomplete type as field + define_class("TraceArg") + .define_constructor(Constructor()) + .define_attr("pp_extra", &TraceArg::ppExtra) + .define_attr("flags", &TraceArg::flags); + + define_global_function("create_extra_data_ptr", &createExtraDataPtr); + define_global_function("destroy_extra_data_ptr", &destroyExtraDataPtr); + define_global_function("get_extra_data_value", &getExtraDataValue); + define_global_function("set_extra_data_value", &setExtraDataValue); + + Module m = define_module("TestingModule"); + + // Test basic struct creation and field access + std::string code = R"(arg = TraceArg.new + arg.flags)"; + + Object result = m.module_eval(code); + ASSERT_EQUAL(0, detail::From_Ruby().convert(result.value())); +} + +TESTCASE(DoublePointerIncompleteAssign) +{ + // Register the incomplete type + define_class("ExtraData2"); + + // Test assigning double pointer to incomplete type + define_class("TraceArg2") + .define_constructor(Constructor()) + .define_attr("pp_extra", &TraceArg::ppExtra); + + define_global_function("create_extra_data_ptr", &createExtraDataPtr); + define_global_function("destroy_extra_data_ptr", &destroyExtraDataPtr); + define_global_function("get_extra_data_value", &getExtraDataValue); + define_global_function("set_extra_data_value", &setExtraDataValue); + + Module m = define_module("TestingModule"); + + // Create double pointer, assign to struct, read back + std::string code = R"(pp = create_extra_data_ptr() + arg = TraceArg2.new + arg.pp_extra = pp + value = get_extra_data_value(arg.pp_extra) + destroy_extra_data_ptr(pp) + value)"; + + Object result = m.module_eval(code); + ASSERT_EQUAL(42, detail::From_Ruby().convert(result.value())); +} + +TESTCASE(DoublePointerIncompleteModify) +{ + // Register the incomplete type + define_class("ExtraData3"); + + // Test modifying through double pointer to incomplete type + define_class("TraceArg3") + .define_constructor(Constructor()) + .define_attr("pp_extra", &TraceArg::ppExtra); + + define_global_function("create_extra_data_ptr", &createExtraDataPtr); + define_global_function("destroy_extra_data_ptr", &destroyExtraDataPtr); + define_global_function("get_extra_data_value", &getExtraDataValue); + define_global_function("set_extra_data_value", &setExtraDataValue); + + Module m = define_module("TestingModule"); + + // Create, assign, modify, read back + std::string code = R"(pp = create_extra_data_ptr() + arg = TraceArg3.new + arg.pp_extra = pp + set_extra_data_value(arg.pp_extra, 999) + value = get_extra_data_value(arg.pp_extra) + destroy_extra_data_ptr(pp) + value)"; + + Object result = m.module_eval(code); + ASSERT_EQUAL(999, detail::From_Ruby().convert(result.value())); } \ No newline at end of file From 067bdf8e45ea7a450a9694681952f859d0132183 Mon Sep 17 00:00:00 2001 From: cfis Date: Mon, 19 Jan 2026 17:39:43 -0800 Subject: [PATCH 6/8] Do a better job handling iterators that return by value. --- rice/detail/NativeIterator.ipp | 7 +++- test/test_Iterator.cpp | 74 +++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/rice/detail/NativeIterator.ipp b/rice/detail/NativeIterator.ipp index ec64b7b5..249e1004 100644 --- a/rice/detail/NativeIterator.ipp +++ b/rice/detail/NativeIterator.ipp @@ -89,7 +89,12 @@ namespace Rice::detail detail::To_Ruby toRuby; for (; it != end; ++it) { - protect(rb_yield, toRuby.convert(*it)); + // Use auto&& to accept both reference- and value-returning iterators. + // - If *it is an lvalue (T&), auto&& deduces to T&, no copy made. + // - If *it is a prvalue (T), auto&& deduces to T&&, value binds to the temporary for the scope of this loop iteration. + // This also avoids MSVC C4239 when convert expects a non-const lvalue reference. + auto&& value = *it; + protect(rb_yield, toRuby.convert(value)); } return self; diff --git a/test/test_Iterator.cpp b/test/test_Iterator.cpp index 49e2d19e..bdc475a0 100644 --- a/test/test_Iterator.cpp +++ b/test/test_Iterator.cpp @@ -40,6 +40,47 @@ namespace uint32_t index; }; + // Iterator that returns by value, not by reference (like cv::FileNodeIterator). + // This tests the fix for MSVC warning C4239. + class ValueReturningIterator + { + public: + using iterator_category = std::input_iterator_tag; + using value_type = Data; + using difference_type = std::ptrdiff_t; + using pointer = Data*; + using reference = Data&; // Note: standard expects reference, but we return by value + + ValueReturningIterator(std::vector* data, size_t index) : data_(data), index_(index) {} + + // Returns by VALUE, not by reference - this is the key characteristic being tested + Data operator*() const { return (*data_)[index_]; } + + ValueReturningIterator& operator++() { ++index_; return *this; } + ValueReturningIterator operator++(int) { auto tmp = *this; ++index_; return tmp; } + bool operator==(const ValueReturningIterator& other) const { return index_ == other.index_; } + bool operator!=(const ValueReturningIterator& other) const { return index_ != other.index_; } + + private: + std::vector* data_; + size_t index_; + }; + + class ContainerWithValueIterator + { + public: + ContainerWithValueIterator() + { + this->data_ = { {10}, {20}, {30} }; + } + + ValueReturningIterator begin() { return ValueReturningIterator(&data_, 0); } + ValueReturningIterator end() { return ValueReturningIterator(&data_, data_.size()); } + + private: + std::vector data_; + }; + class ContainerValues { public: @@ -121,8 +162,10 @@ TEARDOWN(Iterator) { Data_Type::unbind(); Data_Type::unbind(); + Data_Type::unbind(); Rice::detail::Registries::instance.types.remove(); Rice::detail::Registries::instance.types.remove(); + Rice::detail::Registries::instance.types.remove(); rb_gc_start(); } @@ -178,7 +221,7 @@ TESTCASE(Lambda) define_class("ContainerValues") .define_constructor(Constructor()) .include_module(rb_mEnumerable) - .define_method("each", [](VALUE self, VALUE proc)->VALUE + .define_method("each", [](VALUE self, VALUE /*proc*/)->VALUE { if (!detail::protect(rb_block_given_p)) { @@ -472,4 +515,33 @@ TESTCASE(IterateNoCopy) Data_Object actual(a[(long)i]); ASSERT_EQUAL(&expected, &(*actual)); } +} + +// Test iterator that returns by value (like cv::FileNodeIterator). +// This previously caused MSVC warning C4239 about binding rvalue to non-const reference. +TESTCASE(ValueReturningIterator) +{ + define_class("Data") + .define_constructor(Constructor()) + .define_attr("index", &Data::index, Rice::AttrAccess::Read); + + define_class("ContainerWithValueIterator") + .define_constructor(Constructor()) + .define_iterator(&ContainerWithValueIterator::begin, &ContainerWithValueIterator::end); + + Module m = define_module("TestingModule"); + + std::string code = R"(result = [] + container = ContainerWithValueIterator.new + container.each do |x| + result << x.index + end + result)"; + + Array a = m.module_eval(code); + + ASSERT_EQUAL(3, a.size()); + ASSERT_EQUAL(10, detail::From_Ruby().convert(a[0].value())); + ASSERT_EQUAL(20, detail::From_Ruby().convert(a[1].value())); + ASSERT_EQUAL(30, detail::From_Ruby().convert(a[2].value())); } \ No newline at end of file From 19d24e60d5a0d0f6c80bd85b7938cc255e861c65 Mon Sep 17 00:00:00 2001 From: cfis Date: Tue, 20 Jan 2026 22:58:11 -0800 Subject: [PATCH 7/8] Remove the exception handler because it doesn't work. If an exception is raised and we catch it, eventually we have to allocate a Ruby string to throw an exception and then that fails. --- rice/Data_Type.ipp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/rice/Data_Type.ipp b/rice/Data_Type.ipp index 3c323f39..8f029847 100644 --- a/rice/Data_Type.ipp +++ b/rice/Data_Type.ipp @@ -5,16 +5,13 @@ namespace Rice template inline void ruby_mark_internal(detail::WrapperBase* wrapper) { - detail::cpp_protect([&] - { - // Tell the wrapper to mark the objects its keeping alive - wrapper->ruby_mark(); - - // Get the underlying data and call custom mark function (if any) - // Use the wrapper's stored rb_data_type to avoid type mismatch - T* data = static_cast(wrapper->get(Data_Type::ruby_data_type())); - ruby_mark(data); - }); + // Tell the wrapper to mark the objects its keeping alive + wrapper->ruby_mark(); + + // Get the underlying data and call custom mark function (if any) + // Use the wrapper's stored rb_data_type to avoid type mismatch + T* data = static_cast(wrapper->get(Data_Type::ruby_data_type())); + ruby_mark(data); } template From a837214176eeca9d69e68a7f9f2660760fc97eb0 Mon Sep 17 00:00:00 2001 From: cfis Date: Tue, 20 Jan 2026 22:58:51 -0800 Subject: [PATCH 8/8] Unbind classes at test start - before GG would happen after unbinding classes thereby breaking the mark function. --- test/test_Iterator.cpp | 7 ++++--- test/test_Overloads.cpp | 12 ++++++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/test/test_Iterator.cpp b/test/test_Iterator.cpp index bdc475a0..89bc1333 100644 --- a/test/test_Iterator.cpp +++ b/test/test_Iterator.cpp @@ -156,17 +156,18 @@ namespace SETUP(Iterator) { embed_ruby(); -} -TEARDOWN(Iterator) -{ Data_Type::unbind(); Data_Type::unbind(); Data_Type::unbind(); + Rice::detail::Registries::instance.types.remove(); Rice::detail::Registries::instance.types.remove(); Rice::detail::Registries::instance.types.remove(); +} +TEARDOWN(Iterator) +{ rb_gc_start(); } diff --git a/test/test_Overloads.cpp b/test/test_Overloads.cpp index 23195d96..2c6f8bc2 100644 --- a/test/test_Overloads.cpp +++ b/test/test_Overloads.cpp @@ -8,11 +8,6 @@ using namespace Rice; TESTSUITE(Overloads); -SETUP(Overloads) -{ - embed_ruby(); -} - namespace { class MyClass; @@ -25,8 +20,10 @@ namespace class MyClass8; } -TEARDOWN(Overloads) +SETUP(Overloads) { + embed_ruby(); + Data_Type::unbind(); Data_Type::unbind(); Data_Type::unbind(); @@ -44,7 +41,10 @@ TEARDOWN(Overloads) Rice::detail::Registries::instance.types.remove(); Rice::detail::Registries::instance.types.remove(); Rice::detail::Registries::instance.natives.reset(Module(rb_mKernel)); +} +TEARDOWN(Overloads) +{ rb_gc_start(); }