@elelel,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft Open Technologies, Inc.. We will now review your pull request.
Thanks,
Microsoft Open Technologies, Inc. Pull Request Bot

AppVeyor tests apparently use non-existent after VS2017 release RC-version image

hmm.. made changes to the appveyor config file to allow windows builds. Hope that helps..

Isn't filter predicate by nature bool(const T& v) ? If not, we can keep both on_value versions with an overload, though it's not pretty.

Test is a user supplied lambda. It can take anything convertible from v. The forwarding is fine to on_next, but the v passed to test must not use forward. In fact it must be cast to const& instead.

Do you mean just this->test((const Value&)v) ? I thought of:

      template <class Value>
      typename std::enable_if<false == std::is_rvalue_reference<Value>::value, void>::type
      on_next(Value&& v) const {
        auto passed = on_exception([&] () {
            return this->test(v);},
          dest);
        if (!passed.empty() && passed.get()) dest.on_next(v);
      }
      template <class Value>
      typename std::enable_if<true == std::is_rvalue_reference<Value>::value, void>::type 
      on_next(Value&& v) const {
        auto passed = on_exception([&] () {
            return this->test(std::forward(v));},
          dest);
        if (!passed.empty() && passed.get()) dest.on_next(v);
      }

I believe that enable_if would have a bad impact on compile time. I really want to improve compile times. If overloads are the solution, concrete overloads would be better. I think that there would only be 3 (T&, T&& and const T&). However, taking by Value&& and then calling on_next(forward<Value>(v)) seems like an improvement over the existing code.

Do you mean just this->test((const Value&)v) ?

I would like a function like std::move or std::forward that would 'decay' v to a const& rather than a C cast to const T&. Perhaps such a function is defined in the GCL and an implementation could be added to rxcpp::util. Then the call would be something like test(rxu::in(v)). Or perhaps std::reference_wrapper and std::cref can be used to achieve this.

I think that there would only be 3 (T&, T&& and const T&).

Did I understand correctly that on_next(T&& v) and on_next(T& v) overloads were meant here? T&& reference collapsing rule in argument declaration may match an lvalue as T& does, so, if I'm not mistaken, it's potentially an ambiguity error. I'm not sure that all compilers can resolve that identically with a "choose more specific" rule. Or is such case resolution well-defined from standard's point of view?

However, taking by Value&& and then calling on_next(forward(v)) seems like an improvement over the existing code.

Initially I explored the code to make the filter usage faster, and noticed that map operator currently is written with forwarding like in this PR. I think map's and filter's implementations in this part should not be different, in fact they should share the same code like in #363.

I would like a function like std::move or std::forward that would 'decay' v to a const& rather than a C cast to const T&. Perhaps such a function is defined in the GCL and an implementation could be added to rxcpp::util. Then the call would be something like test(rxu::in(v)). Or perhaps std::reference_wrapper and std::cref can be used to achieve this

I will try to look into that, but my feeling is that I'm not familiar enough with RxCpp internals/decaying techniques to write such changes. There's a risk that I write something flawed and potentially not reusable.

I believe that enable_if would have a bad impact on compile time. I really want to improve compile times.

Not concerning this PR, I would suggest to consider refactoring the library structure to improve compile times. We can make the include dependency tree more narrow and change to a multiple-roots include style. It can be modelled more or less like STL: you don't #include <rx.hpp> there, only the explicit parts you really interface with, like #include <vector>. The included file in turn would not internally include convenience include packs, like rx-includes.hpp, but only the dependencies it really needs. That has some downsides, like having to manage dependencies for each single file, but leads to a leaner header-only library.

T&& reference collapsing rule in argument declaration may match an lvalue as T& does

I would expect that source_value_type&& would be more specific match than source_value_type& or const source_value_type& when an r-value is being matched.

I still prefer to take the universal reference and cast to a const Value& using a helper function for test and forward for on_next.

I think map's and filter's implementations in this part should not be different

I agree that both should use forwarding for on_next, but filter must prevent the value from being stolen by the test predicate.

There's a risk that I write something flawed and potentially not reusable.

I see this as a fixed constant in programming :) I prefer to manage this risk via the process of iterate-and-review. I am sure we can solve this. If we can find an existing solution great, otherwise we can do our best and fix bugs as they are discovered. I see review as more of a learning and sharing experience (for everyone, including spectators) rather than a mechanism to prevent bugs. When a review does find a bug, that is serendipitous!

Not concerning this PR, I would suggest to consider refactoring the library structure to improve compile times

Yes, I think it would be useful to have an open issue to discuss this.

Thanks!!

I would expect that source_value_type&& would be more specific match than source_value_type& or const source_value_type& when an r-value is being matched.

I usually followed the reasoning in the second answer here and avoided it: http://stackoverflow.com/questions/9882871/overloaded-function-templates-with-reference-parameters

I still prefer to take the universal reference and cast to a const Value& using a helper function for test and forward for on_next.
...
If we can find an existing solution great, otherwise we can do our best and fix bugs as they are discovered. I see review as more of a learning and sharing experience (for everyone, including spectators) rather than a mechanism to prevent bugs. When a review does find a bug, that is serendipitous!

Unfortunately I don't quite see what exactly is meant by it and what the merits are. If a function is meant, taking a universal ref and casting the argument to const& inside, I don't follow what would be achieved. In my opinion trying to stay non-restrictive in the interface (i.e., not forcing the predicate just to be const & in the public api) is a move in the trade off between the interests of "conscious" library users that are "clean" with their C++ practices and the interests of "relaxed" users who can stick a predicate with a non-const-specific function signature, towards the latter. If I understand the motives correctly, that is to make the usage of the library easier thus enlarging the potential user base. Still the price of it is the complication of development and probably opening ways of getting less compiler-optimizable and erroneous code. In STL filter equivalent, remove_if, they had to make a similar trade-off:

The signature of the predicate function should be equivalent to the following:
bool pred(const Type &a);
The signature does not need to have const &, but the function must not modify the objects passed to it.

Obviously, they had to get all those C-style programmers of the time to be comfortable with the library. The wording of it is somewhat funny itself: the function should behave like it takes const&, but we will not make the compiler fail if you supply it with a func taking something else. With RxCpp however we are in a different situation: it is built not only upon C++ foundations, but the whole functional/immutable/composability ideology. I don't think the choice for the less restrictive interface is so obvious, and frankly I don't think the choice made in STL back then is such a good decision today either. I think enforcing a const& on the predicate in such cases is an element of ensuring that the library is used with logical correctness, not just a way of making library's internals simpler. Because of this, for me it's not just a way to introduce a bug that can then be fixed, it's a step in a direction that is not quite clear to me, with a method I don't clearly understand. Since this PR appeared to touch so many complex things, I suggest we just close it.

I would like to take this improvement. I am willing to go the last mile with this change if you are done.

I found that the meta library in range-v3 has as_const which is close to what I was hoping for, just not sure about the r-value overload being deleted.

Thanks!

I am willing to go the last mile with this change if you are done.

Please do, I'm really lost here with the cast cases.

I found that the meta library in range-v3 has as_const which is close to what I was hoping for, just not sure about the r-value overload being deleted.

The delete is there to explicitly prevent rvalue arguments from passing. That is by N4582 proposal draft they quote in the commit. This discussion may be of interest: http://stackoverflow.com/questions/39051460/why-does-as-const-forbid-rvalue-arguments

merged in #371