Skip to content

Commit

Permalink
Don't copy elements in a container when iterating over them. Fixes #183
Browse files Browse the repository at this point in the history
  • Loading branch information
@cfis
cfis committed Feb 19, 2024
1 parent f31d3f5 commit 0129b88
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 58 deletions.
46 changes: 46 additions & 0 deletions rice/Data_Object.ipp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,52 @@ namespace Rice::detail
Return* returnInfo_ = nullptr;
};

template <typename T>
class To_Ruby<T*&>
{
public:
To_Ruby() = default;

explicit To_Ruby(Return* returnInfo) : returnInfo_(returnInfo)
{
}

VALUE convert(T* data)
{
if (data)
{
// Note that T could be a pointer or reference to a base class while data is in fact a
// child class. Lookup the correct type so we return an instance of the correct Ruby class
std::pair<VALUE, rb_data_type_t*> rubyTypeInfo = detail::Registries::instance.types.figureType(*data);
bool isOwner = this->returnInfo_ && this->returnInfo_->isOwner();
return detail::wrap(rubyTypeInfo.first, rubyTypeInfo.second, data, isOwner);
}
else
{
return Qnil;
}
}

VALUE convert(const T* data)
{
if (data)
{
// Note that T could be a pointer or reference to a base class while data is in fact a
// child class. Lookup the correct type so we return an instance of the correct Ruby class
std::pair<VALUE, rb_data_type_t*> rubyTypeInfo = detail::Registries::instance.types.figureType(*data);
bool isOwner = this->returnInfo_ && this->returnInfo_->isOwner();
return detail::wrap(rubyTypeInfo.first, rubyTypeInfo.second, data, isOwner);
}
else
{
return Qnil;
}
}

private:
Return* returnInfo_ = nullptr;
};

template<typename T>
class To_Ruby<Data_Object<T>>
{
Expand Down
4 changes: 2 additions & 2 deletions rice/detail/NativeIterator.ipp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ namespace Rice::detail
// Hack the enumerator object by storing name_ on the enumerator object so
// the rb_size_function above has access to it
ID method_id = Identifier(this->method_name_).id();
protect(rb_ivar_set, enumerator, rb_intern("rice_method"), method_id );
protect(rb_ivar_set, enumerator, rb_intern("rice_method"), method_id);

return enumerator;
}
Expand All @@ -93,7 +93,7 @@ namespace Rice::detail

for (; it != end; ++it)
{
protect(rb_yield, detail::To_Ruby<Value_T>().convert(*it));
protect(rb_yield, detail::To_Ruby<Value_T&>().convert(*it));
}

return self;
Expand Down
178 changes: 122 additions & 56 deletions test/test_Iterator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ namespace
public:
ContainerValues()
{
this->data_ = { {1}, {2}, {3}};
this->data_ = { {1}, {2}, {3} };
}

std::vector<Data>::iterator begin()
Expand Down Expand Up @@ -134,43 +134,36 @@ namespace
};
}

template <>
struct detail::To_Ruby<Data>
{
static VALUE convert(const Data& data)
{
return detail::to_ruby(data.index);
}
};

template <>
struct detail::To_Ruby<Data*>
{
static VALUE convert(const Data* data)
{
return detail::to_ruby(data->index);
}
};

TESTCASE(iterator_value)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

define_class<ContainerValues>("ContainerValues")
.define_constructor(Constructor<ContainerValues>())
.define_iterator(&ContainerValues::begin, &ContainerValues::end);

ContainerValues* container = new ContainerValues();
Object wrapper = Data_Object<ContainerValues>(container);

Array a = wrapper.instance_eval("a = []; each() { |x| a << x }; a");
Array a = wrapper.instance_eval("each.to_a");
ASSERT_EQUAL(3u, 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]);
Data_Object<Data> wrappedData = a[0];
ASSERT_EQUAL(1, wrappedData->index);

wrappedData = (Data_Object<Data>)a[1];
ASSERT_EQUAL(2, wrappedData->index);

wrappedData = (Data_Object<Data>)a[2];
ASSERT_EQUAL(3, wrappedData->index);
}

TESTCASE(const_iterator_value)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

define_class<ContainerValues>("ContainerValues")
.define_constructor(Constructor<ContainerValues>())
.define_iterator(&ContainerValues::cbegin, &ContainerValues::cend);
Expand All @@ -184,16 +177,23 @@ TESTCASE(const_iterator_value)
end
result)";

Array result = m.module_eval(code);
Array a = m.module_eval(code);

Data_Object<Data> wrappedData = a[0];
ASSERT_EQUAL(1, wrappedData->index);

ASSERT_EQUAL(3u, result.size());
ASSERT_EQUAL(Object(detail::to_ruby(1)), result[0]);
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[1]);
ASSERT_EQUAL(Object(detail::to_ruby(3)), result[2]);
wrappedData = (Data_Object<Data>)a[1];
ASSERT_EQUAL(2, wrappedData->index);

wrappedData = (Data_Object<Data>)a[2];
ASSERT_EQUAL(3, wrappedData->index);
}

TESTCASE(iterator_pointer)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

define_class<ContainerPointers>("ContainerPointers")
.define_constructor(Constructor<ContainerPointers>())
.define_iterator(&ContainerPointers::begin, &ContainerPointers::end);
Expand All @@ -210,16 +210,23 @@ TESTCASE(iterator_pointer)
end
result)";

Array result = m.module_eval(code);
Array a = m.module_eval(code);

Data_Object<Data> wrappedData = a[0];
ASSERT_EQUAL(1, wrappedData->index);

ASSERT_EQUAL(3u, result.size());
ASSERT_EQUAL(Object(detail::to_ruby(1)), result[0]);
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[1]);
ASSERT_EQUAL(Object(detail::to_ruby(3)), result[2]);
wrappedData = (Data_Object<Data>)a[1];
ASSERT_EQUAL(2, wrappedData->index);

wrappedData = (Data_Object<Data>)a[2];
ASSERT_EQUAL(3, wrappedData->index);
}

TESTCASE(two_iterator_pointer)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

define_class<ContainerPointers>("ContainerPointers")
.define_constructor(Constructor<ContainerPointers>())
.define_iterator(&ContainerPointers::begin, &ContainerPointers::end)
Expand All @@ -240,51 +247,110 @@ TESTCASE(two_iterator_pointer)
end
result)";

Array result = m.module_eval(code);
Array a = m.module_eval(code);

ASSERT_EQUAL(6u, a.size());

ASSERT_EQUAL(6u, result.size());
ASSERT_EQUAL(Object(detail::to_ruby(1)), result[0]);
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[1]);
ASSERT_EQUAL(Object(detail::to_ruby(3)), result[2]);
ASSERT_EQUAL(Object(detail::to_ruby(3)), result[3]);
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[4]);
ASSERT_EQUAL(Object(detail::to_ruby(1)), result[5]);
Data_Object<Data> wrappedData = a[0];
ASSERT_EQUAL(1, wrappedData->index);

wrappedData = (Data_Object<Data>)a[1];
ASSERT_EQUAL(2, wrappedData->index);

wrappedData = (Data_Object<Data>)a[2];
ASSERT_EQUAL(3, wrappedData->index);

wrappedData = (Data_Object<Data>)a[3];
ASSERT_EQUAL(3, wrappedData->index);

wrappedData = (Data_Object<Data>)a[4];
ASSERT_EQUAL(2, wrappedData->index);

wrappedData = (Data_Object<Data>)a[5];
ASSERT_EQUAL(1, wrappedData->index);
}

TESTCASE(map)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>())
.define_attr("index", &Data::index, Rice::AttrAccess::Read);

define_class<ContainerPointers>("ContainerPointers")
.define_constructor(Constructor<ContainerPointers>())
.define_iterator(&ContainerPointers::begin, &ContainerPointers::end);

Module m = define_module("Testing");

std::string code = R"(container = ContainerPointers.new
container.map do |x|
x * 2
container.map do |data|
data.index * 2
end)";

Array result = m.module_eval(code);
Array a = m.module_eval(code);
ASSERT_EQUAL(3u, a.size());

Object element = a[0];
ASSERT_EQUAL(2, detail::From_Ruby<int>().convert(element));

ASSERT_EQUAL(3u, result.size());
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[0]);
ASSERT_EQUAL(Object(detail::to_ruby(4)), result[1]);
ASSERT_EQUAL(Object(detail::to_ruby(6)), result[2]);
element = a[1];
ASSERT_EQUAL(4, detail::From_Ruby<int>().convert(element));

element = a[2];
ASSERT_EQUAL(6, detail::From_Ruby<int>().convert(element));
}

TESTCASE(to_enum)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

define_class<ContainerPointers>("ContainerPointers")
.define_constructor(Constructor<ContainerPointers>())
.define_iterator(&ContainerPointers::begin, &ContainerPointers::end);

Module m = define_module("TestingModule");

std::string code = R"(container = ContainerPointers.new
container.each.map do |x|
x * 2
container.each.map do |data|
data.index * 2
end)";

Array result = m.module_eval(code);
Array a = m.module_eval(code);

ASSERT_EQUAL(3u, a.size());

Object element = a[0];
ASSERT_EQUAL(2, detail::From_Ruby<int>().convert(element));

element = a[1];
ASSERT_EQUAL(4, detail::From_Ruby<int>().convert(element));

element = a[2];
ASSERT_EQUAL(6, detail::From_Ruby<int>().convert(element));
}

TESTCASE(IterateNoCopy)
{
define_class<Data>("Data")
.define_constructor(Constructor<Data, uint32_t>());

ASSERT_EQUAL(3u, result.size());
ASSERT_EQUAL(Object(detail::to_ruby(2)), result[0]);
ASSERT_EQUAL(Object(detail::to_ruby(4)), result[1]);
ASSERT_EQUAL(Object(detail::to_ruby(6)), result[2]);
}
define_class<ContainerPointers>("ContainerValues")
.define_constructor(Constructor<ContainerValues>())
.define_iterator(&ContainerPointers::begin, &ContainerPointers::end);

Module m = define_module("Testing");

ContainerValues container;
Data_Object<ContainerValues> wrapper(container);
Array a = wrapper.instance_eval("self.to_a");

ASSERT_EQUAL(container.data_.size(), a.size());

for (int i = 0; i < container.data_.size(); i++)
{
Data& expected = container.data_[i];
Data_Object<Data> actual(a[i]);
ASSERT_EQUAL(&expected, &(*actual));
}
}

0 comments on commit 0129b88

Please sign in to comment.