boostorg / hana

Your standard library for metaprogramming
http://boostorg.github.io/hana
Boost Software License 1.0
1.69k stars 215 forks source link

Add support for member function to member_ptr #449

Open sdebionne opened 5 years ago

sdebionne commented 5 years ago

This PR adds an overload of operator() of member_ptr to support member function so that the following use case is now supported:

namespace ns {
    struct Person {
        std::string name;
        int age;

        void init(std::string name_arg, int age_arg) {
            age = age_arg;
            name = name_arg;
        };

        bool is_old() const { return age > 30; };
    };
}

BOOST_HANA_ADAPT_STRUCT(ns::Person,
    name,
    age,
    init,
    is_old
);

// The member names are hana::strings:
auto names = hana::transform(hana::accessors<ns::Person>(), hana::first);
BOOST_HANA_CONSTANT_ASSERT(
    names == hana::make_tuple(BOOST_HANA_STRING("name"), BOOST_HANA_STRING("age"), BOOST_HANA_STRING("init"), BOOST_HANA_STRING("is_old"))
);

int main() {
    ns::Person john{ "John", 30 };

    constexpr auto accessors = hana::accessors<ns::Person>();
    // Call john.init()
    hana::second(accessors[hana::size_c<2>])(john, "Johnny", 31);
    // Call john.is_old()
    auto old = hana::second(accessors[hana::size_c<3>])(john);
}

Could you give me some guidance about adding unit tests? I had a look in test/concept/struct but I am not sure where the new tests belongs...

sdebionne commented 5 years ago

For symmetry with the member object version, it maybe be better to have member_ptr returning a binded member function that would be used something like:

// Call john.init()
hana::second(accessors[hana::size_c<2>])(john)("Johnny", 31);
                                              ^ member call

An other advantage of this solution is that one can introspect the type of the member functions in the same way than the member objects, e.g .

template <typename S>
constexpr auto types = decltype(hana::unpack(std::declval<S>(),
      hana::on(hana::make_tuple, hana::compose(hana::typeid_, hana::second)))){};

I have an implementation but that uses C++20 std::bind_front().

ricejasonf commented 5 years ago

Why does it need std::bind_front? Would a lambda suffice, and how would it capture the object? by reference? That might be the only issue here is that the user needs to know of possible dangling or copying.

For tests, I'd say test/concept/struct/macro.* files are the appropriate place to test this. Just add a member function to the test Data struct and check that it can be called.

Your statement about "symmetry" is correct. The interface to accessors should not change. What you are modifying is a "data-type" that implements the Struct concept.

Currently the instance generated by the macros explodes if you give it a member function so this appears it would be a backwards compatible improvement provided you don't modify the interface to accessors.

Here is your above code with the unmodified use of accessors against Boost 1.70. So I think this confirms member functions currently don't work with the macro. https://godbolt.org/z/aCTQ00

sdebionne commented 5 years ago

@ricejasonf Thanks for your feedback and for the GodBolt code, this is how I should have started this PR.

Why does it need std::bind_front? Would a lambda suffice and how would it capture the object?

I did not mean to say that std::bind_front is required, actually a variadic lambda would do, I guess, see last commit. Regarding the capture, my immediate answer would be by reference.

Ultimately I would like the function object returned by member_ptr to be compatible with Boost.CallableTraits args, see contraints, and both solutions fail to comply with the requirements. I need to do some research to see if this is possible with a little bit of meta-programming.

For now, I reverted my code to return the member function pointer (that breaks the symmetry, but works with CallableTraits):

template <typename T, std::enable_if_t<std::is_member_function_pointer_v<Memptr>, int> = 0>
constexpr decltype(auto) operator()(T &&) const
{ return ptr; }

Boost.Hana is not really a reflection library, I believe it's more a side effect, so I don't know if this PR is pushing in the right direction...