Revamp handling of ruby arguments - use two passes instead of one. The first pass extracts passed in Ruby args and the second fills in default values per different native functions/methods.

Author: cfis

rice/Arg.hpp

@@ -83,7 +83,6 @@ namespace Rice
 
   public:
     std::string name;
-    int32_t position = -1;
 
   private:
     //! Our saved default value

rice/detail/InstanceRegistry.hpp

@@ -1,8 +1,6 @@
 #ifndef Rice__detail__InstanceRegistry__hpp_
 #define Rice__detail__InstanceRegistry__hpp_
 
-#include <map>
-
 namespace Rice::detail
 {
   class InstanceRegistry

rice/detail/Native.hpp

@@ -42,8 +42,8 @@ namespace Rice::detail
     void operator=(const Native&) = delete;
     void operator=(Native&&) = delete;
 
-    virtual Resolved matches(size_t argc, const VALUE* argv);
-    virtual VALUE operator()(size_t argc, const VALUE* argv, VALUE self) = 0;
+    virtual Resolved matches(std::map<std::string, VALUE>& values);
+    virtual VALUE operator()(std::map<std::string, VALUE>& values, VALUE self) = 0;
     virtual std::string toString() = 0;
 
     // Ruby API access
@@ -56,7 +56,8 @@ namespace Rice::detail
     template<typename T, bool isBuffer>
     static void verify_type();
 
-    std::vector<std::optional<VALUE>> getRubyValues(size_t argc, const VALUE* argv, bool validate);
+    static std::map<std::string, VALUE> readRubyArgs(size_t argc, const VALUE* argv);
+    std::vector<std::optional<VALUE>> getRubyValues(std::map<std::string, VALUE> values, bool validate);
     ParameterAbstract* getParameterByName(std::string name);
     Convertible matchParameters(std::vector<std::optional<VALUE>>& values);
 

rice/detail/Native.ipp

@@ -65,6 +65,8 @@ namespace Rice::detail
     // Execute the function but make sure to catch any C++ exceptions!
     return cpp_protect([&]()
     {
+      std::map<std::string, VALUE> values = readRubyArgs(argc, argv);
+
       const std::vector<std::unique_ptr<Native>>& natives = Registries::instance.natives.lookup(klass, methodId);
 
       if (natives.size() == 1)
@@ -86,7 +88,7 @@ namespace Rice::detail
           std::back_inserter(resolves), 
           [&](const std::unique_ptr<Native>& native)
           {
-            return native->matches(argc, argv);
+            return native->matches(values);
           });
 
         // Now sort from best to worst
@@ -146,7 +148,7 @@ namespace Rice::detail
       }
 
       // Call the C++ function
-      return (*native)(argc, argv, self);
+      return (*native)(values, self);
     });
   }
 
@@ -290,14 +292,8 @@ namespace Rice::detail
     // Fill in missing args
     for (size_t i = argsVector.size(); i < std::tuple_size_v<Parameter_Tuple>; i++)
     {
-      argsVector.emplace_back(std::make_unique<Arg>("arg_" + std::to_string(i)));
-    }
-
-    // TODO - there has to be a better way!
-    for (size_t i = 0; i < argsVector.size(); i++)
-    {
-      std::unique_ptr<Arg>& arg = argsVector[i];
-      arg->position = (int32_t)i;
+      std::string argName = "arg_" + std::to_string(i);
+      argsVector.emplace_back(std::make_unique<Arg>(argName));
     }
 
     auto indices = std::make_index_sequence<std::tuple_size_v<Parameter_Tuple>>{};
@@ -328,81 +324,116 @@ namespace Rice::detail
     return result;
   }
 
-  inline std::vector<std::optional<VALUE>> Native::getRubyValues(size_t argc, const VALUE* argv, bool validate)
+  inline std::map<std::string, VALUE> Native::readRubyArgs(size_t argc, const VALUE* argv)
   {
-#undef max
-    size_t size = std::max(this->parameters_.size(), argc);
-    std::vector<std::optional<VALUE>> result(size);
+    std::map<std::string, VALUE> result;
 
     // Keyword handling
     if (rb_keyword_given_p())
     {
       // Keywords are stored in the last element in a hash
       size_t actualArgc = argc - 1;
 
-      VALUE value = argv[actualArgc];
-      Hash keywords(value);
-
       // Copy over leading non-keyword arguments
       for (size_t i = 0; i < actualArgc; i++)
       {
-        result[i] = argv[i];
+        std::string key = "arg_" + std::to_string(i);
+        result[key] = argv[i];
       }
 
+      VALUE value = argv[actualArgc];
+      Hash keywords(value);
+
       // Copy over keyword arguments
       for (auto pair : keywords)
       {
-        Symbol key(pair.first);
-        ParameterAbstract* parameter = this->getParameterByName(key.str());
-        if (!parameter && validate)
-        {
-          throw std::invalid_argument("Unknown keyword: " + key.str());
-        }
-        else if (!parameter)
-        {
-          continue;
-        }
-
-        const Arg* arg = parameter->arg();
-
-        result[arg->position] = pair.second.value();
+        result[pair.first.to_s().str()] = pair.second.value();
       }
     }
     else
     {
-      std::copy(argv, argv + argc, result.begin());
+      // Copy over leading non-keyword arguments
+      for (size_t i = 0; i < argc; i++)
+      {
+        std::string key = "arg_" + std::to_string(i);
+        result[key] = argv[i];
+      }
     }
 
     // Block handling. If we find a block and the last parameter is missing then
     // set it to the block
-    if (rb_block_given_p() && result.size() > 0 && !result.back().has_value())
+    if (rb_block_given_p())// FIXME && result.size() > 0 && !result.back().second.has_value())
     {
       VALUE proc = rb_block_proc();
-      result.back() = proc;
+      std::string key = "arg_" + std::to_string(result.size());
+      result[key] = proc;
     }
 
-    if (validate)
+    return result;
+  }
+
+  inline std::vector<std::optional<VALUE>> Native::getRubyValues(std::map<std::string, VALUE> values, bool validate)
+  {
+    // !!!NOTE!!! We copied the values parameter because we are going to modify it!
+
+    // Protect against user sending too many arguments
+    if (values.size() > this->parameters_.size())
+    {
+      std::string message = "wrong number of arguments (given " +
+        std::to_string(values.size()) + ", expected " + std::to_string(this->parameters_.size()) + ")";
+      throw std::invalid_argument(message);
+    }
+
+    std::vector<std::optional<VALUE>> result(this->parameters_.size());
+
+    for (size_t i=0; i< this->parameters_.size(); i++)
     {
-      // Protect against user sending too many arguments
-      if (argc > this->parameters_.size())
+      std::unique_ptr<ParameterAbstract>& parameter = this->parameters_[i];
+      Arg* arg = parameter->arg();
+
+      // If using keywords arguments, then the value key will be arg->name(). If using positional 
+      // arguments then they key will be "arg_<position>"
+      std::string keywordKey = arg->name;
+      std::string positionKey = "arg_" + std::to_string(i);
+
+      auto iter = values.find(keywordKey);
+      if (iter == values.end() && keywordKey != positionKey)
       {
-        std::string message = "wrong number of arguments (given " +
-          std::to_string(argc) + ", expected " + std::to_string(this->parameters_.size()) + ")";
-        throw std::invalid_argument(message);
+        iter = values.find(positionKey);
       }
 
-      for (size_t i = 0; i < result.size(); i++)
+      if (iter != values.end())
+      {
+        result[i] = iter->second;
+        // Remove the value
+        values.erase(iter);
+      }
+      else if (arg->hasDefaultValue())
+      {
+        result[i] = parameter->defaultValueRuby();
+      }
+      else if (validate)
       {
-        std::optional<VALUE> value = result[i];
-        ParameterAbstract* parameter = this->parameters_[i].get();
+        std::string message = "Missing argument. Name: " + arg->name + ". Index: " + std::to_string(i) + ".";
+        throw std::invalid_argument(message);
+      }
+    }
 
-        if (!parameter->arg()->hasDefaultValue() && !value.has_value())
-        {
-          std::string message;
-          message = "Missing argument. Name: " + parameter->arg()->name + ". Index: " + std::to_string(parameter->arg()->position) + ".";
-          throw std::invalid_argument(message);
-        }
+    // Check for unknown arguments
+    if (validate && values.size() > 0)
+    {
+      // There are unknown arguments
+      std::ostringstream message;
+      message << "Unknown argument(s): ";
+      size_t count = 0;
+      for (const std::pair<const std::string, VALUE>& pair : values)
+      {
+        if (count > 0)
+          message << ", ";
+        message << pair.first;
+        count++;
       }
+      throw std::invalid_argument(message.str());
     }
 
     return result;
@@ -420,25 +451,20 @@ namespace Rice::detail
     return result;
   }
 
-  inline Resolved Native::matches(size_t argc, const VALUE* argv)
+  inline Resolved Native::matches(std::map<std::string, VALUE>& values)
   {
     // Return false if Ruby provided more arguments than the C++ method takes
-    if (argc > this->parameters_.size())
+    if (values.size() > this->parameters_.size())
       return Resolved{ Convertible::None, 0, this };
 
     Resolved result{ Convertible::Exact, 1, this };
 
-    std::vector<std::optional<VALUE>> rubyValues = this->getRubyValues(argc, argv, false);
+    std::vector<std::optional<VALUE>> rubyValues = this->getRubyValues(values, false);
     result.convertible = this->matchParameters(rubyValues);
 
     if (this->parameters_.size() > 0)
     {
-      size_t providedValues = std::count_if(rubyValues.begin(), rubyValues.end(), [](std::optional<VALUE>& value)
-      {
-        return value.has_value();
-      });
-
-      result.parameterMatch = providedValues / (double)this->parameters_.size();
+      result.parameterMatch = values.size() / (double)this->parameters_.size();
     }
     return result;
   }

rice/detail/NativeAttributeGet.hpp

@@ -35,8 +35,8 @@ namespace Rice
       void operator=(const NativeAttribute_T&) = delete;
       void operator=(NativeAttribute_T&&) = delete;
 
-      Resolved matches(size_t argc, const VALUE* argv) override;
-      VALUE operator()(size_t argc, const VALUE* argv, VALUE self) override;
+      Resolved matches(std::map<std::string, VALUE>& values) override;
+      VALUE operator()(std::map<std::string, VALUE>& values, VALUE self) override;
       std::string toString() override;
 
       NativeKind kind() override;

rice/detail/NativeAttributeGet.ipp

@@ -27,9 +27,9 @@ namespace Rice::detail
   }
 
   template<typename Attribute_T>
-  inline Resolved NativeAttributeGet<Attribute_T>::matches(size_t argc, const VALUE*)
+  inline Resolved NativeAttributeGet<Attribute_T>::matches(std::map<std::string, VALUE>& values)
   {
-    if (argc == 0)
+    if (values.size() == 0)
       return Resolved { Convertible::Exact, 1, this };
     else
       return Resolved{ Convertible::None, 0, this };
@@ -43,7 +43,7 @@ namespace Rice::detail
   }
 
   template<typename Attribute_T>
-  inline VALUE NativeAttributeGet<Attribute_T>::operator()(size_t, const VALUE*, VALUE self)
+  inline VALUE NativeAttributeGet<Attribute_T>::operator()(std::map<std::string, VALUE>&, VALUE self)
   {
     if constexpr (std::is_member_object_pointer_v<Attribute_T>)
     {

rice/detail/NativeAttributeSet.hpp

@@ -26,8 +26,8 @@ namespace Rice
       void operator=(const NativeAttribute_T&) = delete;
       void operator=(NativeAttribute_T&&) = delete;
 
-      Resolved matches(size_t argc, const VALUE* argv) override;
-      VALUE operator()(size_t argc, const VALUE* argv, VALUE self) override;
+      Resolved matches(std::map<std::string, VALUE>& values) override;
+      VALUE operator()(std::map<std::string, VALUE>& values, VALUE self) override;
       std::string toString() override;
 
       NativeKind kind() override;

rice/detail/NativeAttributeSet.ipp

@@ -31,23 +31,23 @@ namespace Rice::detail
   }
 
   template<typename Attribute_T>
-  inline Resolved NativeAttributeSet<Attribute_T>::matches(size_t argc, const VALUE*)
+  inline Resolved NativeAttributeSet<Attribute_T>::matches(std::map<std::string, VALUE>& values)
   {
-    if (argc == 1)
+    if (values.size() == 1)
       return Resolved{ Convertible::Exact, 1, this };
     else
       return Resolved{ Convertible::None, 0, this };
   }
 
   template<typename Attribute_T>
-  inline VALUE NativeAttributeSet<Attribute_T>::operator()(size_t argc, const VALUE* argv, VALUE self)
+  inline VALUE NativeAttributeSet<Attribute_T>::operator()(std::map<std::string, VALUE>& values, VALUE self)
   {
-    if (argc != 1)
+    if (values.size() != 1)
     {
       throw std::runtime_error("Incorrect number of parameters for setting attribute. Attribute: " + this->name_);
     }
 
-    VALUE value = argv[0];
+    VALUE value = values.begin()->second;
 
     if constexpr (!std::is_null_pointer_v<Receiver_T>)
     {

rice/detail/NativeCallback.hpp

@@ -51,7 +51,7 @@ namespace Rice::detail
     static inline NativeCallback_T* native_;
 
   private:
-    VALUE operator()(size_t argc, const VALUE* argv, VALUE self) override;
+    VALUE operator()(std::map<std::string, VALUE>& values, VALUE self) override;
     std::string toString() override;
     NativeKind kind() override;
     VALUE returnKlass() override;

rice/detail/NativeCallback.ipp

@@ -243,7 +243,7 @@ namespace Rice::detail
   }
 
   template<typename Return_T, typename ...Parameter_Ts>
-  inline VALUE NativeCallback<Return_T(*)(Parameter_Ts...)>::operator()(size_t, const VALUE*, VALUE)
+  inline VALUE NativeCallback<Return_T(*)(Parameter_Ts...)>::operator()(std::map<std::string, VALUE>&, VALUE)
   {
     return Qnil;
   }