ruby-rice / rice

Ruby Interface for C++ Extensions
http://ruby-rice.github.io/
Other
370 stars 62 forks source link

`Arg` of type `shared_ptr` ignores default value #191

Closed kvtb closed 9 months ago

kvtb commented 11 months ago

If Arg has type shared_ptr<...> it ignores default value.

That seems just unimplemented in From_Ruby<std::shared_ptr<T>> compared to other From_Ruby<...>

Is it intentional?

kvtb commented 11 months ago

I fixed it with

diff --git a/rice/stl/smart_ptr.ipp b/rice/stl/smart_ptr.ipp
index f310e49..4d09307 100644
--- a/rice/stl/smart_ptr.ipp
+++ b/rice/stl/smart_ptr.ipp
@@ -93,8 +93,20 @@ namespace Rice::detail
   class From_Ruby<std::shared_ptr<T>>
   {
   public:
+    explicit From_Ruby()
+    {
+    }
+
+    explicit From_Ruby(Arg * arg) : arg_(arg)
+    {
+    }
+
     std::shared_ptr<T> convert(VALUE value)
     {
+      if (value == Qnil && this->arg_ && this->arg_->hasDefaultValue())
+      {
+        return this->arg_->defaultValue<std::shared_ptr<T>>();
+      }
       Wrapper* wrapper = detail::getWrapper(value, Data_Type<T>::ruby_data_type());

       using Wrapper_T = WrapperSmartPointer<std::shared_ptr, T>;
@@ -106,14 +118,28 @@ namespace Rice::detail
       }
       return smartWrapper->data();
     }
+  private:
+    Arg* arg_ = nullptr;
   };

   template <typename T>
   class From_Ruby<std::shared_ptr<T>&>
   {
   public:
+    explicit From_Ruby()
+    {
+    }
+
+    explicit From_Ruby(Arg * arg) : arg_(arg)
+    {
+    }
+
     std::shared_ptr<T>& convert(VALUE value)
     {
+      if (value == Qnil && this->arg_ && this->arg_->hasDefaultValue())
+      {
+        return this->arg_->defaultValue<std::shared_ptr<T>>();
+      }
       Wrapper* wrapper = detail::getWrapper(value, Data_Type<T>::ruby_data_type());

       using Wrapper_T = WrapperSmartPointer<std::shared_ptr, T>;
@@ -125,6 +151,8 @@ namespace Rice::detail
       }
       return smartWrapper->data();
     }
+  private:
+    Arg* arg_ = nullptr;
   };

   template<typename T>
diff --git a/test/test_Stl_SmartPointer.cpp b/test/test_Stl_SmartPointer.cpp
index 7236f8f..9873927 100644
--- a/test/test_Stl_SmartPointer.cpp
+++ b/test/test_Stl_SmartPointer.cpp
@@ -109,6 +109,9 @@ SETUP(SmartPointer)
   define_global_function("extract_flag_unique_ptr_ref", &extractFlagUniquePtrRef);
   define_global_function("extract_flag_shared_ptr", &extractFlagSharedPtr);
   define_global_function("extract_flag_shared_ptr_ref", &extractFlagSharedPtrRef);
+
+  define_global_function("extract_flag_shared_ptr_def", &extractFlagSharedPtr, Arg("myClass") = std::make_shared<MyClass>());
+  define_global_function("extract_flag_shared_ptr_ref_def", &extractFlagSharedPtrRef, Arg("myClass") = std::make_shared<MyClass>());
 }

 TESTCASE(TransferOwnership)
@@ -197,4 +200,32 @@ TESTCASE(SharedPtrRefParameter)
                         extract_flag_shared_ptr_ref(my_class))";

   Object result = m.module_eval(code);
-}
\ No newline at end of file
+}
+
+TESTCASE(SharedPtrDefaultParameter)
+{
+  MyClass::reset();
+
+  Module m = define_module("TestingModule");
+
+  std::string code = R"(factory = Factory.new
+                        my_class = factory.share
+                        my_class.set_flag(7)
+                        extract_flag_shared_ptr_def())";
+
+  Object result = m.module_eval(code);
+}
+
+TESTCASE(SharedPtrRefDefaultParameter)
+{
+  MyClass::reset();
+
+  Module m = define_module("TestingModule");
+
+  std::string code = R"(factory = Factory.new
+                        my_class = factory.share
+                        my_class.set_flag(7)
+                        extract_flag_shared_ptr_ref_def())";
+
+  Object result = m.module_eval(code);
+}
jasonroelofs commented 11 months ago

I doubt it was intentional, there's just a lot of combinations when you pull in the STL and this didn't get covered. Thanks for the diff, I'll take a look.

jasonroelofs commented 9 months ago

Thanks for the report and the patch! This was just something we missed, I've updated shared_ptr support accordingly.