billyquith/ponder

Invalid values returned by ArrayProperty::get with std::vector<bool>

AntAgna opened this issue · 5 comments

I am trying to use std::vector<> with Ponder.

If I have a std::vector of bool and I use ArrayProperty::get(const UserObject& object, std::size_t index), I receive invalid values.
The same thing with a std::vector of int works correctly.
I attach example code.

Another note : The compiler generates warning C4172 (returning address of local variable or temporary) when a std::vector is declared.
It is as if the vector was returned by value at some point instead of by reference.
I am wondering if it may be due to a missing template specialization or to a bug in the compiler.

I am building using Visual Studio 2015.
I will try another compiler tomorrow.

main.zip

The problem also happens with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

The warning message is more explicit :

In file included from include/ponder/valuemapper.hpp:38:0,
                 from include/ponder/value.hpp:37,
                 from include/ponder/tagholder.hpp:36,
                 from include/ponder/property.hpp:35,
                 from include/ponder/arrayproperty.hpp:35,
                 from main.cpp:4:
include/ponder/arraymapper.hpp: In instantiation of ‘static const T& ponder_ext::ArrayMapper<std::vector<_RealType> >::get(const std::vector<_RealType>&, std::size_t) [with T = bool; std::size_t = long unsigned int]’:
include/ponder/detail/arraypropertyimpl.inl:68:23:   required from ‘ponder::Value ponder::detail::ArrayPropertyImpl<A>::getElement(const ponder::UserObject&, std::size_t) const [with A = ponder::detail::Accessor1<ExampleStruct, std::vector<bool>&, void>; std::size_t = long unsigned int]’
main.cpp:77:1:   required from here
include/ponder/arraymapper.hpp:258:25: warning: returning reference to temporary [-Wreturn-local-addr]
         return arr[index];
                         ^

When running the program built by gcc, the result is : Segmentation fault (core dumped)

Aha, I found the cause.

std::vector<bool> is a specialization of std::vector<T>.
In this specialization, array subscript does not return a reference : typedef bool const_reference;
This is documented here : http://en.cppreference.com/w/cpp/container/vector_bool

We need to return bool instead of bool& in ArrayMapper::get()

#include <ponder/classget.hpp>
#include <ponder/errors.hpp>
#include <ponder/arrayproperty.hpp>
#include <ponder/class.hpp>
#include <ponder/classbuilder.hpp>

struct ExampleStruct
{
	static void declare();	// For introspection

	std::vector<int> Ints = { 1, 2, 3, 4 };
	std::vector<bool> Bools = { false, true, false, true };
};

PONDER_AUTO_TYPE(ExampleStruct, &ExampleStruct::declare)	// Will automatically call declare()

inline void ExampleStruct::declare()
{
	// Declare structure members
	ponder::Class::declare< ExampleStruct >("ExampleStruct")
		.property("Ints", &ExampleStruct::Ints)
		.property("Bools", &ExampleStruct::Bools)				// This causes the following warning:  C4172: returning address of local variable or temporary
		;
}

int main()
{
	ExampleStruct Instance;

	const ponder::Class& Class = ponder::classByType<ExampleStruct>();
	ponder::UserObject Object(Instance);

	{
		// Test with int array
		const ponder::Property& Prop = Class.property("Ints");
		const ponder::ArrayProperty& ArrayProp = dynamic_cast<const ponder::ArrayProperty&>(Prop);

		std::vector<int> Copy;

		for (size_t i = 0; i < ArrayProp.size(Object); i++)
		{
			ponder::Value value = ArrayProp.get(Object, i);
			int v = value.to<int>();
			Copy.push_back(v);
		}

		assert(Copy == ExampleStruct().Ints);		// Works correctly
	}

	{
		// Test with bool array
		const ponder::Property& Prop = Class.property("Bools");
		const ponder::ArrayProperty& ArrayProp = dynamic_cast<const ponder::ArrayProperty&>(Prop);

		std::vector<bool> Copy;

		for (size_t i = 0; i < ArrayProp.size(Object); i++)
		{
			ponder::Value value = ArrayProp.get(Object, i);
			bool v = value.to<bool>();
			Copy.push_back(v);
		}

		assert(Copy == ExampleStruct().Bools);		// Fails here (Copy is all true)
	}
	return 0;
}

Good, work. Thanks for looking into this.

Specialization for bool

The Standard Library defines a specialization of the vector template for bool. The description of this specialization indicates that the implementation should pack the elements so that every bool only uses one bit of memory.[8] This is widely considered a mistake.[9][10] vector does not meet the requirements for a C++ Standard Library container. For instance, a container::reference must be a true lvalue of type T. This is not the case with vector::reference, which is a proxy class convertible to bool.[11] Similarly, the vector::iterator does not yield a bool& when dereferenced. There is a general consensus among the C++ Standard Committee and the Library Working Group that vector should be deprecated and subsequently removed from the standard library, while the functionality will be reintroduced under a different name.[12]

from wikipedia.

Also: A Specification to deprecate vector