Compile error when using transparent function objects about functionalplus HOT 35 CLOSED

I was playing around yesterday, and after refactoring my POC I stumbled on the compiler error you mentioned in test_tree.cpp.

It is because I only handled &F::operator(), which only works for function objects.
I was thinking about putting is_invocable/invoke et al. in the library to help with this, which can be implemented in C++11.

You might want to replace result_of with invoke_result later on, the former has been deprecated in C++17, and the Notes on cppreference are quite scary.

I will also need to workaround the template arguments that rely on result_of. The issue is that those are used to determine the return type of the function.
Right now, I use a fallback to return a invalid_call_t type when result_of cannot retrieve a Callable return type, that way I am able to trigger static_asserts in the body of zip_with.

Without this workaround, none of the static_asserts are triggered, which is quite inconvenient.

I think I might be able to open a PR before the end of the week, which will only handle zip_with. If you approve the changes, I will do the other functions as well :)

from functionalplus.

Perfect, it shouldn't take that long to implement

from functionalplus.

I believe this is has been superseded by the trigger_static_asserts facility.

I will see if it can be removed once I finish converting the library.

from functionalplus.

Thank you. That is a very good observation. I can reproduce the problem with g++ and clang:

#include <iostream>
#include <vector>
#include <string>

#include <fplus/fplus.hpp>

using namespace std::string_literals;

int main()
{
    auto const l1 = {"Hello"s, "World"s};
    auto const l2 = {","s, "!"s};
    auto const v = fplus::zip_with(std::plus<>{}, l1, l2);
    std::cout << fplus::show_cont_with(" ", v) << std::endl;
}

g++ 5.4:

$ g++ -std=c++14 issue79.cpp 
In file included from /usr/local/include/fplus/compare.hpp:9:0,
                 from /usr/local/include/fplus/fplus.hpp:9,
                 from issue79.cpp:5:
/usr/local/include/fplus/function_traits.hpp: In instantiation of ‘struct utils::function_traits<std::plus<void> >’:
/usr/local/include/fplus/pairs.hpp:42:52:   required from ‘ContainerOut fplus::zip_with(F, const ContainerIn1&, const ContainerIn2&) [with ContainerIn1 = std::initializer_list<const std::__cxx11::basic_string<char> >; ContainerIn2 = std::initializer_list<const std::__cxx11::basic_string<char> >; F = std::plus<void>; X = const std::__cxx11::basic_string<char>; Y = const std::__cxx11::basic_string<char>; TOut = std::__cxx11::basic_string<char>; ContainerOut = std::vector<std::__cxx11::basic_string<char> >]’
issue79.cpp:13:55:   required from here
/usr/local/include/fplus/function_traits.hpp:78:8: error: decltype cannot resolve address of overloaded function
 struct function_traits
        ^
In file included from /usr/local/include/fplus/numeric.hpp:11:0,
                 from /usr/local/include/fplus/generate.hpp:10,
                 from /usr/local/include/fplus/container_properties.hpp:11,
                 from /usr/local/include/fplus/fplus.hpp:12,
                 from issue79.cpp:5:
/usr/local/include/fplus/pairs.hpp: In instantiation of ‘ContainerOut fplus::zip_with(F, const ContainerIn1&, const ContainerIn2&) [with ContainerIn1 = std::initializer_list<const std::__cxx11::basic_string<char> >; ContainerIn2 = std::initializer_list<const std::__cxx11::basic_string<char> >; F = std::plus<void>; X = const std::__cxx11::basic_string<char>; Y = const std::__cxx11::basic_string<char>; TOut = std::__cxx11::basic_string<char>; ContainerOut = std::vector<std::__cxx11::basic_string<char> >]’:
issue79.cpp:13:55:   required from here
/usr/local/include/fplus/pairs.hpp:42:52: error: ‘arity’ is not a member of ‘utils::function_traits<std::plus<void> >’
     static_assert(utils::function_traits<F>::arity == 2,
                                                    ^
/usr/local/include/fplus/pairs.hpp:45:71: error: no class template named ‘arg’ in ‘struct utils::function_traits<std::plus<void> >’
     typedef typename utils::function_traits<F>::template arg<0>::type FIn0;
                                                                       ^
/usr/local/include/fplus/pairs.hpp:46:71: error: no class template named ‘arg’ in ‘struct utils::function_traits<std::plus<void> >’
     typedef typename utils::function_traits<F>::template arg<1>::type FIn1;
                                                                       ^
/usr/local/include/fplus/pairs.hpp:71:18: error: no class template named ‘arg’ in ‘struct utils::function_traits<std::plus<void> >’
     return result;
                  ^
/usr/local/include/fplus/pairs.hpp:71:18: error: no class template named ‘arg’ in ‘struct utils::function_traits<std::plus<void> >’

clang 3.8:

$ clang++ -std=c++14 issue79.cpp

In file included from issue79.cpp:5:
In file included from /usr/local/include/fplus/fplus.hpp:9:
In file included from /usr/local/include/fplus/compare.hpp:9:
/usr/local/include/fplus/function_traits.hpp:79:39: error: reference to overloaded function could not be resolved; did you mean to call it?
    : public function_traits<decltype(&T::operator())>
                                      ^~~~~~~~~~~~~~
/usr/local/include/fplus/pairs.hpp:42:26: note: in instantiation of template class 'utils::function_traits<std::plus<void> >' requested here
    static_assert(utils::function_traits<F>::arity == 2,
                         ^
issue79.cpp:13:25: note: in instantiation of function template specialization 'fplus::zip_with<std::initializer_list<std::__cxx11::basic_string<char> >, std::initializer_list<std::__cxx11::basic_string<char> >, std::plus<void>,
      std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > > >' requested here
  auto const v = fplus::zip_with(std::plus<>{}, l1, l2);
                        ^
1 error generated.

However I also did not use stuff like std::plus<void> yet. But I will see what I can find out. :-)

from functionalplus.

The problem is the following: In order to produce better error compiler error messages, fplus often checks the arity of the functions given to higher-order functions.

It also does not work with generic lambdas:

#include <iostream>
#include <vector>
#include <string>

#include <fplus/fplus.hpp>

int main()
{
    const std::vector<std::string> l1 = {"Hello", "World"};
    const std::vector<std::string> l2 = {",", "!"};
    const auto lambda_plus_generic = [](const auto& x, const auto& y)
    {
        return x + y;
    };
    fplus::zip_with(lambda_plus_generic, l1, l2);
}

$ clang++ -std=c++14 issue79.cpp 
In file included from issue79.cpp:5:
In file included from /usr/local/include/fplus/fplus.hpp:9:
In file included from /usr/local/include/fplus/compare.hpp:9:
/usr/local/include/fplus/function_traits.hpp:79:39: error: reference to overloaded function could not be resolved; did you mean to call it?
    : public function_traits<decltype(&T::operator())>
                                      ^~~~~~~~~~~~~~
/usr/local/include/fplus/pairs.hpp:42:26: note: in instantiation of template class 'utils::function_traits<(lambda at issue79.cpp:11:38)>' requested here
    static_assert(utils::function_traits<F>::arity == 2,
                         ^
issue79.cpp:15:12: note: in instantiation of function template specialization 'fplus::zip_with<std::vector<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >,
      std::vector<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > >, (lambda at issue79.cpp:11:38), std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char>,
      std::vector<std::__cxx11::basic_string<char>, std::allocator<std::__cxx11::basic_string<char> > > >' requested here
    fplus::zip_with(lambda_plus_generic, l1, l2);
           ^
1 error generated.

Assuming this to be a similar problem to the one with the transparent function object you provided, I continue to discuss this version.

Deducing the arity of a generic lambda seems to be quite difficult. So currently it looks like some kind of tradeoff to me. Supporting generic functions vs. nicer compiler error messages.

Additionally the return type of such a function needs to be known to instantiate a type for the container returned by zip_with (and others). I agree that this advanced kind of type-deduction would be nice, since I also like it from other functional languages. In an ideal world we could emulate type inference like in Elm etc. here in C++. But right now I do not know how we could move further into this direction with this library, especially since I would like the keep the core (leaving fplus::fwd and fplus::curry aside) C++11 compatible.

Any ideas? :-)

from functionalplus.

Hi, good findings!

It seems impossible to conciliate both indeed, that's quite unfortunate for generic lambdas to be honest, I use them quite a lot (infinitely more that transparent function objects :)).

You could only sacrifice C++14 users by disabling the arity checks when C++14 is on, since both aforementioned features are not present before this version.
That's not a good solution, I agree!

Or this could be an opt-in?

From what I've read on the SO link you posted, one cannot retrieve the arity of a function object, since the operator() could be variadic.
Hence, why not just checking if the call if well-formed? You could add a static_assert and then go check the arity so it appears in the compile errors?

from functionalplus.

How about doing something like

template<typename F, typename X, typename Y>
void foo(F f, X x, Y y)
{
   decltype(f(x, y), void())(); // Dear programmer: compiler-error on this line indicates that your f(x,y) is not type-correct

    // ..
}

from functionalplus.

If I understand correctly, this kind of check already is implemented in the following line.

    typedef typename std::result_of<F(X, Y)>::type FOut;

So leaving out the following lines renders zip_with compatible with generic lambdas and also with std::plus<>{}.

    static_assert(utils::function_traits<F>::arity == 2,
        "Function must take two parameters.");
    typedef typename utils::function_traits<F>::template arg<0>::type FIn0;
    typedef typename utils::function_traits<F>::template arg<1>::type FIn1;
    typedef typename ContainerIn1::value_type T1;
    typedef typename ContainerIn2::value_type T2;
    static_assert(std::is_convertible<T1, FIn0>::value,
        "Function does not take elements from first Container as first Parameter.");
    static_assert(std::is_convertible<T2, FIn1>::value,
        "Function does not take elements from second Container as second Parameter.");

And actually these checks of course also are done implicitly in the mentioned line using result_of, just with compiler error messages perhaps not that obvious.

But I guess supporting generic lambdas will be more important in the long run.

Do you guys have a better idea than simply remove these checks? Otherwise I will go through the whole library and delete all this stuff in order to support generic lambdas etc. everywhere.

from functionalplus.

I think there can be a middleground, I'll post the code once it works (or if I yield)

from functionalplus.

Well, unfortunately I cannot SFINAE out the instantiation of utils::function_traits<GenericLambda>, the compiler just gives me a hard error.

If someones knows out to implement a has_function_traits trait helper, I'm all ears, this could solve the problem.

By transforming function_traits like this:

template <typename T, typename ...Args>
struct function_traits
    : public function_traits<decltype(&T::template operator()<Args...>)>
{};

It works with generic lambdas, with a small caveat: You must get the Args correctly, which is at best combinatory and tedious, but my bet would be on impossible.

We could keep all those checks, they are really helpful, but we should add a static_assert before to help generic lambdas users:

template <std::size_t N> struct priority_tag : priority_tag<N - 1> {};
template <> struct priority_tag<0> {};

template <typename F, typename... Args>
auto detect_call(priority_tag<1>, F &&f, Args &&... args)
    -> decltype(std::forward<F>(f)(std::forward<Args>(args)...),
                std::true_type{});

template <typename F, typename... Args>
std::false_type detect_call(priority_tag<0>, F &&, Args &&...);

template <typename F, typename... Args>
constexpr bool is_valid_call(F &&f, Args &&... args) {
  return decltype(detect_call(priority_tag<1>{}, std::forward<F>(f),
                         std::forward<Args>(args)...))::value;
}

// .....

static_assert(is_valid_call(...),
"Your call is invalid, I hope for you that you're not using generic lambdas,
it's gonna rain errors");

from functionalplus.

Sorry for the late reply, but I think I do not understand what you wrote.

If the second half of your comment is a suggestion for how to solve the problem, would you mind writing it out fully for zip_with?

from functionalplus.

Hello, sorry my message was unclear. I managed to implement zip_with support for generic lambdas, while keeping the static_asserts.

Here is the patch (renamed to .txt since github doesn't support .patch):

generic-lambdas.txt

The code I wrote is very messy and only work for zip_with, the error messages need to be improved. I'm sere there are better metaprogramming way to achieve this, but I didn't want to take too much time on it this morning :).

I could refine it later though.

You can modify the test.cpp to use generic lambdas, it should work!

from functionalplus.

Thanks for the clarification. I applied your patch and added something to ignore warnings.

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
[detect_call code]
#pragma GCC diagnostic pop

It mostly works really fine, but the unit tests (namely tree_test.cpp) do not compile. Do you think you could have a look at it? You can build and run the tests as follows:

mkdir build
cd build
cmake -DUNITTEST=ON -DCPP14TEST=ON ..
make unittest

from functionalplus.

Sure, I'll take a look tonight, would be great to make it really generic, and add a mechanism to reduce boilerplate for each algorithm.

I'll let you know!

from functionalplus.

Yes, that would be nice. Having to split every function foo into foo, foo_impl and foo_impl<false> would increase the size of the library code quite a bit and decrease its readability as far as it currently looks to me.

from functionalplus.

Yes that was just a POC, I'm sure there's a better way to do this. It might require some macros but I'll figure it out. Hopefully ;)

from functionalplus.

That sounds awesome! I really appreciate all the effort you are putting in. 👍

from functionalplus.

@theodelrieu, are you still interested in working on the other functions additional to zip_with as well?

from functionalplus.

Sure, is there anything special to do with fwd::* functions?

from functionalplus.

I think they should work out of the box. The tests for fwd::zip_with I just added work fine.

from functionalplus.

Well, it will take a while after all, there is a lot of functions to change.
I will work on it on my free time, it might be ready next week-end.

from functionalplus.

There's no rush. Take all the time you need.

from functionalplus.

Hi, I'll have some difficulties to make atomic commits... There is a LOT of function_traits checks in the library, but the real issue is that some of them are needed to determine the return type (e.g. every method that returns a lambda).

In order to compile those parts of the code, we have to use auto trailing return types everywhere (good thing we upgraded to C++14 :D).

This is a big performance improvement too, no std::function will be returned anymore.

from functionalplus.

This sound good. Don't worry too much about the commits being atomic. It is nice if they are, but in my opinion the end result is more important. So feel free to open a PR with one large commit. ;)

from functionalplus.

Cool, I also tried to make most functions SFINAE-friendly, unfortunately that is incompatible with the static_asserts. That will be an issue for people wishing to use something like this:

// F being a callable that will call a `fplus` function when invoked
if constexpr(!std::is_invocable_v<F, Args...>) {
  // do something special
}

Since fplus methods will not be SFINAE-friendly, this will fail to compile if Args... are wrongs, because is_invocable will instantiate the body of the fplus function and trigger static_asserts and so on...

But that's already the case, that's not a regression. Plus, I guess this use-case is not really the top-priority of the library :)

from functionalplus.

Yes, I guess we currently can neglect this use case. :)

from functionalplus.

Hi, I stumbled upon bind_1st* functions earlier, and I remembered what you said in #83 (that fwd::* methods should be used instead).

Should we remove them then? (No, it's not just because I'm too lazy to convert them... :D)

EDIT: Same thing with bind_unary, it is only used in tests.

from functionalplus.

In general I totally agree with you. I also would prefer to get rid of it, or perhaps replace it with something more modern (probably variadic). The thing is the code base in our company probably is littered with fplus::bind_1st*. :D
I will have a look at this during the next days and talk to my colleagues about it. I guess in the end I could replace it completely in one afternoon. It would not be the first time I adjusted everything to an API change of fplus. ;)

Now since we have C++14 as a general requirement for fplus it totally makes sense to remove bind_1st*t.

bind_unary should be totally superfluous since lazy covers it completely.

from functionalplus.

Great! I could see a variadic version of this function, that would bind the first N arguments, however that is not as flexible as std::bind, and it seems a bit brittle IMO. I will convert those functions then, and let you remove them if/when you want.

from functionalplus.

I just removed bind_* as a test and sifted through the compiler errors. There are some uses of it in fplus itself:

fplus/container_common.hpp:        fplus::bind_1st_of_2(
fplus/container_common.hpp:        auto eqToX = bind_1st_of_2(p, x);
fplus/container_common.hpp:    auto unaryPredicate = bind_1st_of_2(p, xs.front());
fplus/filter.hpp:    const auto pred = bind_2nd_of_2(is_elem_of<ContainerElems>, elems);
fplus/generate.hpp:            bind_1st_of_2(
fplus/maps.hpp:            bind_1st_of_2(transform_snd<Key, InVal, F>, f),
fplus/maps.hpp:    return map_keep_if(bind_2nd_of_2(is_elem_of<KeyContainer>, keys), map);
fplus/maps.hpp:    return map_drop_if(bind_2nd_of_2(is_elem_of<KeyContainer>, keys), map);
fplus/maps.hpp:        bind_2nd_of_2(get_from_map<MapType>, key), maps);
fplus/replace.hpp:    return replace_if(bind_1st_of_2(is_equal<T>, source), dest, xs);
fplus/transform.hpp:    return transform_and_concat(bind_1st_of_2(replicate<T, Container>, n), xs);
fplus/tree.hpp:        const auto find_pred = bind_1st_of_2(tree_is_child_of, *it);
fplus/tree.hpp:                bind_1st_of_2(is_not_equal<int>, 0),

bind_1st_of_ can of course be replaced by fwd:: functions but bind_2nd_of* can not. Also our company's code base uses bind_2nd_of* in a few cases where it reads quite well. So I guess based on this new information I would like to keep these functions in fplus for now.

Thanks for converting them. :-)

from functionalplus.

One small question I had about the internal namespace:

There is a check_arity function, isn't that redundant with the function_traits::arity?

from functionalplus.

internal::check_arity<2, F>();

maps to

template<int TargetArity, typename F>
void check_arity()
{
    internal::check_arity_helper<TargetArity, F>(
        std::integral_constant<bool, check_callable<F>::value>());
}

and that maps to

template<int TargetArity, typename F>
void check_arity_helper(std::true_type)
{
    static_assert(utils::function_traits<F>::arity == TargetArity,
        "Wrong arity.");
}

template<int TargetArity, typename F>
void check_arity_helper(std::false_type)
{
}

So yeah, it looks a bit too convoluted. Instead of writing

internal::check_arity<2, F>();

for example, we could in princible immediately write

static_assert(utils::function_traits<F>::arity == TargetArity, "Wrong arity.");

However for some reason I wanted to ignore the check (i.e. dispatch to check_arity_helper(std::false_type) by using check_callable) in case F does not have a non-template call operator. I am not 100% sure why this was the case or if it is still relevant, but I guess it had to do with user-friendly compiler errors.

If I remember correctly if somebody wrote

const auto res = fplus::generate_by_idx<std::vector<int>>(1, 10);

instead of something meaningful, the error messages looked better this way.

However I just tested it and it seems to no longer be the case.

Replacing

   internal::check_arity<1, F>();

with

    static_assert(utils::function_traits<F>::arity == 1, "Wrong arity.");

in generate_by_idx did not make a difference.

So the only advantage of using internal::check_arity<1, F>(); instead of static_assert(utils::function_traits<F>::arity == 1, "Wrong arity."); seems to be the shorter line of code.

Or do you have an idea why this dispatching could still be helpful?

from functionalplus.

I have an issue with memoize_recursive. I think I cannot make it work with generic lambdas in an acceptable manner.

At some point I need to know the return type of the Callable parameter, which has a continuation (another Callable) as its first parameter, to deduce MemoMap.

What are the requirements on the continuation callback? If we require it to be a template argument and specify which parameters/return type it must have, I could make it work.

from functionalplus.

I think it is totally OK if memoize_recursive does not work with generic lambdas at all.

from functionalplus.

fixed with PR 104

from functionalplus.