8

I would like to implement a function drop_if. Given a unary predicate and a sequential container, it returns a container of the same type holding only the elements from the original one not fulfilling the predicate.

If the input container is an r-value it should work in-place otherwise create a copy. This is achieved by dispatching to the appropriate version in namespace internal. The r-value version should be disabled if the value_type of the container can not be overwritten - like std::pair<const int, int> for example - even if the container is an r-value.

The following code works as expected with clang and current versions of gcc (>=6.3).

#include <algorithm>
#include <iostream>
#include <iterator>
#include <type_traits>
#include <utility>
#include <vector>

namespace internal
{
    template <typename Pred, typename Container,
        typename = typename std::enable_if<
        std::is_assignable<
            typename Container::value_type&,
            typename Container::value_type>::value>::type>
    Container drop_if( Pred pred, Container&& xs )
    {
        std::cout << "r-value" << std::endl;
        xs.erase( std::remove_if( std::begin( xs ), std::end( xs ), pred ), std::end( xs ) );
        return std::move( xs );
    }

    template <typename Pred, typename Container>
    Container drop_if( Pred pred, const Container& xs )
    {
        std::cout << "l-value" << std::endl;
        Container result;
        auto it = std::back_inserter( result );
        std::remove_copy_if( std::begin( xs ), std::end( xs ), it, pred );
        return result;
    }
} // namespace internal

template <typename Pred, typename Container,
    typename Out = typename std::remove_reference<Container>::type>
    Out drop_if( Pred pred, Container&& xs )
{
    return std::move( internal::drop_if( pred, std::forward<decltype(xs)>( xs ) ) );
}

typedef std::pair<int, int> pair_t;
typedef std::vector<pair_t> vec_t;

bool sum_is_even( pair_t p )
{
    return (p.first + p.second) % 2 == 0;
}

typedef std::pair<const int, int> pair_c_t;
typedef std::vector<pair_c_t> vec_c_t;

bool sum_is_even_c( pair_c_t p)
{
    return (p.first + p.second) % 2 == 0;
}

int main()
{
    vec_c_t v_c;
    drop_if( sum_is_even_c, v_c ); // l-value
    drop_if( sum_is_even_c, vec_c_t() ); // l-value

    vec_t v;
    drop_if( sum_is_even, v ); // l-value
    drop_if( sum_is_even, vec_t() ); // r-value
}

However it does not compile on MSVC++ and GCC 6.2, because they behave incorrectly for std::is_assignable:

using T = std::pair<const int, int>;
const auto ok = std::is_assignable<T&, T>::value;
// ok == true on GCC 6.2 and MSVC++

See answer to this question and Library Defect Report 2729.

I would like it to work with different containers and with different kinds of objects, e.g. std::vector<double>, std::map<int, std::string> etc. The std::map case (using a different inserter) is the situation in which I encountered the problem with value_types of std::pair<const T, U>.

Do you have any ideas how the dispatch / sfinae could be changed to also work for MSVC++ (ver. MSVC++ 2017 15.2 26430.6 in my case) and for GCC 6.2 downwards?

Tobias Hermann
  • 7,574
  • 4
  • 37
  • 93
  • This seems like [Library Defect Report 2729](http://cplusplus.github.io/LWG/lwg-defects.html#2729), which might not yet have been implemented everywhere. – Bo Persson Jun 14 '17 at 11:40
  • @BoPersson Yes, T.C. also linked to this in his [answer](https://stackoverflow.com/a/44540292/1866775) to the question linked in this question here. Now I am looking for a workaround. – Tobias Hermann Jun 14 '17 at 12:29
  • 1
    First thing I'd do is I'd use tag dispatching. `internal::drop_if` takes either `true_type` or `false_type` as its first argument. `::drop_if` passes `std::is_assignable<...>{}` to `internal::drop_if`. MSVC's SFINAE support is flakey, and you don't need SFINAE here, just dispatching. Then we can attack `is_assignable` problem directly. (decltype in SFINAE makes MSVC die; decltype in tag dispatching does not) – Yakk - Adam Nevraumont Jun 14 '17 at 13:40
  • 1
    Also `return xs;` -- you need to move or forward here. – Yakk - Adam Nevraumont Jun 14 '17 at 13:42
  • @Yakk: Why do I need to more of forward on `return xs;`? Isn't this [happening automatically](https://stackoverflow.com/a/14856553/1866775)? Edit: Oh, I now understand. It is not superfluous in that case. Thank you. I edited my question accordingly. Also, sorry I do not understand your other comment. Could you please show what you mean? – Tobias Hermann Jun 14 '17 at 15:02
  • [Live this](http://coliru.stacked-crooked.com/a/ee84e0c798bd97e0) -- we make the decision to reuse explicitly in `::drop_if`, then tag-dispatch to the correct `internal::drop_if`. This eliminates SFINAE, which MSVC can have problems with. – Yakk - Adam Nevraumont Jun 14 '17 at 15:43
  • @Yakk Very nice. Thanks a lot! Do you also have an idea on how to circumvent the problem with `std::is_assignable` on [MSVC](http://rextester.com/AZQHH14935) and on [older GCCs](https://godbolt.org/g/vZUrMd)? – Tobias Hermann Jun 14 '17 at 16:01
  • Handling map as motivation is wrong. The functions will only handle sequential containers. – Eugene Jun 15 '17 at 01:53
  • @Eugene Why should the l-value version of `drop_if` not work with `std::map`? – Tobias Hermann Jun 15 '17 at 06:04
  • Because map does not have back_inserter. – Eugene Jun 15 '17 at 23:36
  • Ah, yes, sorry. You are right of course. In my minimal example in the question I did not mention that I use something like `auto itOut = internal::get_back_inserter(ys);` in my real code and `internal::get_back_inserter` is overloaded to different containers, e.g.: https://gist.github.com/Dobiasd/2f0d2472e5f30e44442a8232a171be4b – Tobias Hermann Jun 16 '17 at 06:09

4 Answers4

2

The problem appears to be that MSVC's std::pair<const T, U>::operator= is not SFINAE disabled. It exists even if instantiating it does not work.

So when you detect if it exists, it exists. If you execute it, it fails to compile.

We can work around this. But it is a workaround.

namespace internal
{
    template <typename Pred, typename Container>
    Container drop_if( std::true_type reuse_container, Pred pred, Container&& xs )
    {
        std::cout << "r-value" << std::endl;
        xs.erase( std::remove_if( std::begin( xs ), std::end( xs ), pred ), std::end( xs ) );
        return std::forward<Container>( xs );
    }

    template <typename Pred, typename Container>
    Container drop_if( std::false_type reuse_container, Pred pred, const Container& xs )
    {
        std::cout << "l-value" << std::endl;
        Container result;
        auto it = std::back_inserter( result );
        std::remove_copy_if( std::begin( xs ), std::end( xs ), it, pred );
        return result;
    }
} // namespace internal

template<bool b>
using bool_k = std::integral_constant<bool, b>;

template<class T>
struct can_self_assign {
    using type = std::is_assignable<T&, T>;
};

template<class T>
using can_self_assign_t = typename can_self_assign<T>::type;

template<class T0, class T1>
struct can_self_assign<std::pair<T0, T1>>
{
    enum { t0 = can_self_assign_t<T0>::value, t1 = can_self_assign_t<T1>::value, x = t0&&t1 };
    using type = bool_k< x >;
};

template<>
struct can_self_assign<std::tuple<>>
{
    using type = bool_k< true >;
};
template<class T0, class...Ts>
struct can_self_assign<std::tuple<T0, Ts...>>
{
    using type = bool_k< can_self_assign_t<T0>::value && can_self_assign_t<std::tuple<Ts...>>::value >;
};


template <typename Pred, typename Container,
    typename Out = typename std::remove_reference<Container>::type>
    Out drop_if( Pred pred, Container&& xs )
{
    using dContainer = typename std::decay<Container>::type;
    using can_assign = can_self_assign_t<typename dContainer::value_type>;
    using cannot_reuse = std::is_lvalue_reference<Container>;

    using reuse = std::integral_constant<bool, can_assign::value && !cannot_reuse::value >;

    return internal::drop_if( reuse{}, pred, std::forward<Container>( xs ) );
}

live example and other live example.

I also changed your SFINAE dispatching to tag based dispatching.

Other types with defective operator= disabling may also need can_self_assign specializations. Notable examples may include tuple<Ts...> and vector<T,A> and similar.

I don't know when and if compilers where required to have operator= "not exist" if it wouldn't work in std types; I remember it was not required at one point for std::vector, but I also remember a proposal adding such requirements.

Yakk - Adam Nevraumont
  • 235,777
  • 25
  • 285
  • 465
  • That is very nice. Thanks a lot. One question though: Is there a specific reason for using `return std::forward(xs);` instead of `return std::move>(xs);`? – Tobias Hermann Jun 15 '17 at 08:13
  • 1
    @tobais policy; I forward from forwarding references. Currently it is always an rvalue, but in theory it need not be. Feel free to move *and* static assert we have an rvalue. – Yakk - Adam Nevraumont Jun 15 '17 at 09:50
0

You don't say what version of the Visual C++ compiler you are using but have you tried using trailing return syntax?

In other words, replacing this:

template <typename Pred, typename Container,
          typename Out = typename std::remove_reference<Container>::type>
Out drop_if( Pred pred, Container&& xs )

with something like this?

template <typename Pred, 
          typename Container>
auto drop_if(Pred pred, Container&& xs) -> std::remove_reference<Container>::type

The Visual C++ compiler has gradually added in C++ 11/14/17 features. I've struggled with making the template MPL work and have had to work-around a few things that "should" work but don't.

I will admit this is a bit of a guess but you might give it a try.

Joe
  • 2,355
  • 8
  • 24
  • Thanks for the suggestion. I added my MSVC++ version to my question. Sadly your solution [does not help either](http://rextester.com/XORPQ17442). The linked test btw. uses "Microsoft (R) C/C++ Optimizing Compiler Version 19.00.23506 for x86." – Tobias Hermann Jun 14 '17 at 14:29
  • Bummer sorry. Sounds like you are using Visual Studio 2015. I think that's the one with that compiler version. C++ conformance is pretty good in that one. Edit -- oh you said it's 2017. Then I'm really surprised this was a problem for you. But glad you got a solution. – Joe Jun 15 '17 at 15:40
0

If you want your code to be dedicated specifically to containers of tuple like objects you could do it like this (brutal but working on older versions of gcc as well as on MSVC):

#include <algorithm>
#include <iostream>
#include <iterator>
#include <type_traits>
#include <utility>
#include <vector>

namespace internal
{
    template <class... Ts>
        int foo(Ts&&... ts);

    template <typename Pred, typename Container, std::size_t... Is>
    auto drop_if( Pred pred, Container&& xs, std::index_sequence<Is...>) -> decltype(foo(std::declval<typename std::tuple_element<Is, typename std::remove_reference<typename Container::value_type>::type>::type&>() = std::declval<typename std::tuple_element<Is, typename std::remove_reference<typename Container::value_type>::type>::type>()...),  xs)
    {
        std::cout << "r-value" << std::endl;
        xs.erase( std::remove_if( std::begin( xs ), std::end( xs ), pred ), std::end( xs ) );
        return xs;
    }

    template <typename Pred, typename Container, class I>
    Container drop_if( Pred pred, const Container& xs, I )
    {
        std::cout << "l-value" << std::endl;
        Container result;
        auto it = std::back_inserter( result );
        std::remove_copy_if( std::begin( xs ), std::end( xs ), it, pred );
        return result;
    }
} // namespace internal

template <typename Pred, typename Container,
    typename Out = typename std::remove_reference<Container>::type>
    Out drop_if( Pred pred, Container&& xs )
{
    return internal::drop_if( pred, std::forward<decltype(xs)>( xs ), std::make_index_sequence<std::tuple_size<typename Out::value_type>::value>{} );
}

typedef std::pair<int, int> pair_t;
typedef std::vector<pair_t> vec_t;

bool sum_is_even( pair_t p )
{
    return (p.first + p.second) % 2 == 0;
}

typedef std::pair<const int, int> pair_c_t;
typedef std::vector<pair_c_t> vec_c_t;

bool sum_is_even_c( pair_c_t p)
{
    return (p.first + p.second) % 2 == 0;
}

int main()
{
    vec_c_t v_c;
    drop_if( sum_is_even_c, v_c ); // l-value
    drop_if( sum_is_even_c, vec_c_t() ); // l-value

    vec_t v;
    drop_if( sum_is_even, v ); // l-value
    drop_if( sum_is_even, vec_t() ); // r-value
}

[live demo]

W.F.
  • 13,347
  • 2
  • 29
  • 70
0

Nice finding, until those changes go to be fixed, I offer my solution for this corner case.

namespace internal {
template <typename T>
struct my_is_pair {
  static constexpr bool value = false;
};

template <typename K, typename V>
struct my_is_pair<std::pair<K, V>> {
  static constexpr bool value = true;
};

template <typename T, typename U, typename TIsPair = void>
struct my_is_assignable : std::is_assignable<T, U> {
  using is_pair_type = std::false_type;
};

template <typename T, typename U>
struct my_is_assignable<T,
                        U,
                        typename std::
                          enable_if<my_is_pair<typename std::decay<T>::type>::value
                                    && my_is_pair<typename std::decay<U>::type>::value>::
                            type>
  : std::integral_constant<bool,
                           std::is_assignable<
                             typename std::remove_reference<T>::type::first_type&,
                             const typename std::remove_reference<U>::type::first_type&>::
                               value
                             && std::is_assignable<
                                  typename std::remove_reference<T>::type::second_type&,
                                  const typename std::remove_reference<U>::type::
                                    second_type&>::value> {
  using is_pair_type = std::true_type;
};

template <
  typename Pred,
  typename Container,
  typename = typename std::
    enable_if<my_is_assignable<
                typename std::remove_reference<Container>::type::value_type,
                typename std::remove_reference<Container>::type::value_type>::value
              && std::is_rvalue_reference<Container&&>::value>::type>
Container drop_if(Pred pred, Container&& xs) {
  std::cout << "r-value" << std::endl;
  xs.erase(std::remove_if(std::begin(xs), std::end(xs), pred), std::end(xs));
  return xs;
}

template <typename Pred, typename Container>
Container drop_if(Pred pred, const Container& xs) {
  std::cout << "l-value" << std::endl;
  Container result;
  auto it = std::back_inserter(result);
  std::remove_copy_if(std::begin(xs), std::end(xs), it, pred);
  return result;
}
} // namespace internal

template <typename Pred,
          typename Container,
          typename Out = typename std::remove_reference<Container>::type>
Out drop_if(Pred pred, Container&& xs) {
  return internal::drop_if(pred, std::forward<decltype(xs)>(xs));
}

typedef std::pair<int, int> pair_t;
typedef std::vector<pair_t> vec_t;

bool sum_is_even(pair_t p) { return (p.first + p.second) % 2 == 0; }

typedef std::pair<const int, int> pair_c_t;
typedef std::vector<pair_c_t> vec_c_t;

bool sum_is_even_c(pair_c_t p) { return (p.first + p.second) % 2 == 0; }

int main() {
  vec_c_t v_c;
  drop_if(sum_is_even_c, v_c); // l-value
  drop_if(sum_is_even_c, vec_c_t()); // r-value

  vec_t v;
  drop_if(sum_is_even, v); // l-value
  drop_if(sum_is_even, vec_t()); // r-value
}

This just introduces an is_assignable specialization for the pair type as it is suggested in the defect report. The other thing I added is std::is_rvalue_reference to prevent if from the calls on lvalue references (in your solution it was disabled by a failure substitution in Container::value_type, that fails when Container is vector<...>&.

Yuki
  • 3,234
  • 2
  • 20
  • 30