billyquith / ponder

C++ reflection library with Lua binding, and JSON and XML serialisation.
http://billyquith.github.io/ponder/
Other
640 stars 93 forks source link

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

Closed AntAgna closed 7 years ago

AntAgna commented 7 years ago

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

AntAgna commented 7 years ago

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)

AntAgna commented 7 years ago

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()

billyquith commented 7 years ago
#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;
}
billyquith commented 7 years ago

Good, work. Thanks for looking into this.

billyquith commented 7 years ago

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