imap function applied multiple times about cppitertools HOT 12 CLOSED

Yeah I think I see what you're saying. I'll dig into this tomorrow and let
you know what I come up with. thanks for the example
On Feb 14, 2015 1:53 PM, "staticlibs" [email protected] wrote:

I am trying to translate this (simplified) example from python (2.7) to
C++ (tried both clang 3.4 and gcc 4.7) using move-only objects.

Python snippet, prints 41 43:

import itertools as it

class MyItem:
def init(self, val):
self.val = val
def with_val(self, val):
self.val = val
return self

if "main" == name:
vec = [MyItem(41), MyItem(42), MyItem(43)]
transformed1 = it.imap(lambda el: el.with_val(el.val + 10), vec)
filtered = it.ifilter(lambda el: 52 != el.val, transformed1)
transformed2 = it.imap(lambda el: el.with_val(el.val - 10), filtered)

for el in transformed2:
    print(el.val)

C++ snippet, prints 51 53:

#include
#include

#include "imap.hpp"
#include "filter.hpp"

class MyMoveOnly {
int val;
public:
MyMoveOnly() { };
MyMoveOnly(int val) : val(val) { }
MyMoveOnly(const MyMoveOnly&) = delete;
MyMoveOnly& operator=(const MyMoveOnly&) = delete;
MyMoveOnly(MyMoveOnly&& other) : val(other.val) { }
MyMoveOnly& operator=(MyMoveOnly&& other) {
this->val = other.val;
return *this;
}
int get_val() {
return val;
}
void set_val(int val) {
this->val = val;
}
};

namespace it = iter;

int main() {
// source data
std::vector vec{};
vec.emplace_back(MyMoveOnly(41));
vec.emplace_back(MyMoveOnly(42));
vec.emplace_back(MyMoveOnly(43));
// some transformations
auto transformed1 = it::imap([](MyMoveOnly& el) {
int va = el.get_val();
el.set_val(va + 10);
return std::move(el);
}, vec);
// r-value ref is used here because l-value ref won't compile
auto filtered = it::filter([](MyMoveOnly&& el) {
return 52 != el.get_val();
}, transformed1);
auto transformed2 = it::imap([](MyMoveOnly& el) {
int va = el.get_val();
el.set_val(va - 10);
return std::move(el);
}, filtered);
// print data
for (auto&& el : transformed2) {
std::cout << el.get_val() << std::endl;
}

return 0;

}

I wasn't sure that this example will compile (because of non-copyable
objects), it actually compiles and runs, but the first transformation is
applied twice for each element. I am not sure whether this is intended
behaviour, but it is definitely contr-intuitive because single foreach over
the transformed sequence caused multiple applyings of imap function.

—
Reply to this email directly or view it on GitHub
#18.

from cppitertools.

As I'm transforming this I wanna just give you some tips (feel free to ignore of course):

emplace_back takes arguments to the constructor, so you would likely want to have vec.emplace_back(41); instead.

lambda functions will deduce their returns to be by value. The way you have this set up, it will invalidate the source vector in the general case. You could tell the lambda to return an lvalue reference though

auto inc_ten = [](MyMoveOnly& el) -> MyMoveOnly& {
    int va = el.get_val();
    el.set_val(va + 10);
    return el; 
};

In other news: I know what the problem is and I believe it will affect others as well suh as dropwhile and takewhile. Currently figuring out the best solution

from cppitertools.

As I'm transforming this I wanna just give you some tips (feel free to ignore of course):

I think these tips are quite good and will help other users to workaround the problem.

But I am playing with my own implementation of lazy imap/filter for move-only objects for some time. My library is not finished (and most probably won't be ever finished to publishable condition), but it makes me feel myself like a "poweruser" in iterator wrappers. So please took everithing below as a nitpicking. BTW, your library is much better then boost.range adaptors.

emplace_back takes arguments to the constructor, so you would likely want to have vec.emplace_back(41); instead.

Yeah, but for movable objects this is more or less the same as emplace_back(MyMoveOnly(41)). But calling emplace with bare arguments looks similar to implicit constructors (the language feature is different, but results are similar) and I try to use explicit constructors (and "explicit" emplace) whenever possible.

lambda functions will deduce their returns to be by value. The way you have this set up, it will invalidate the source vector in the general case.

Actually it won't "invalidate" neither the vector itself nor its elements. Vector elements moved from will reside "in a valid but unspecified state". Well-behaved objects should expect to be used after move. And if vector was passed to me by non-const ref modifying its contents looks like normal operation. If it is passed by const ref - it won't compile.

You could tell the lambda to return an lvalue reference though

This is possible only if imap in and out types are the same. But often it is required to transform input object into another type (create new objects in transformation function) and references won't work there, but returning by value will still work.

Currently figuring out the best solution

I actually wasn't expect my examples to work, I was playing with move-only objects to be sure that your library won't support them. I think not supporting non-copyable (move-only) objects in generic iterator wrappers is absolutely fine, because STL-conforming iterator elements must be copyable. And this issue can be viewed as undefined behaviour caused by non-conforming element types. Though, "double apply" feels not good even for UB and compile-time error will be cleaner (correct work will be event better of course).

End of nitpicking, and thanks for the great library, I think itertools is one of the best parts of Python stdlib, was using its JS port some years ago and it is great to have itertools in C++.

from cppitertools.

for movable objects this is more or less the same as emplace_back(MyMoveOnly(41))

It'll be a move construction in that case. Can you tell me why you'd use emplace_back instead of push_back if you're going for explicitness?

Actually it won't "invalidate" neither the vector itself nor its elements. Vector elements moved from will reside "in a valid but unspecified state"

Yes you're right. better explained I would say it would leave the source vector in an undesirable state? If you had something like a vector of unique_ptrs, moving all (or some) of them out of the vector would leave you with a pretty useless vector. That's all I was getting at.

I actually wasn't expect my examples to work

I actually think they should be supported. The problem is that filter dereferences an iterator twice, once when it checks to see if it passes the predicate, and once when it actually "yields" the element. I hadn't consider modifying functions. The real problem is that dereferencing twice only works on forward iterators (and up) and My goal is to support input iterators.

from cppitertools.

About push_back - no specific reason, just trying to avoid operations that requires copy (l-value push_back), and r-value push_back is a no-op, it just calls emplace_back with its argument.

About move - it was just a nitpicking, use-after-move is a grey area, well-behaved objects are not popular (unique_ptrs are not well-behaved). "move-from-source" iterator wrappers are much easier to implement though (no reference wrappers etc).

For experimental "move-from-source" filter I use unique_ptr field inside iterator wrapper, move from source into unique_ptr, then use value from unique_ptr (for default-constructible elements plain value temp field can be used instread of unique_ptr). This filter is hardly convenient though because it requires two functions - one for checks and one for failed elements.

from cppitertools.

I think we're on the same page on most of this. I made a bunch of changes last night and this morning to filter and to further support move-only objects. I have to make the same changes to dropwhile and takewhile that I did to filter, and should be pushing it all out by tomorrow.

from cppitertools.

I think I've solved the multiple dereferencing issues, the issue I have is with the filter function is that the object needs to outlive the call to the lambda, so it doesn't work taking an rvalue reference. If you mark get_val() as const and have the filter function instead take const MyMovable& then your example works as expected. Tell me if this seems right to you

from cppitertools.

Yeah, first example works fine now, thanks.

Stumbled upon another problem (maybe it is better to close this one and to create another issue), this snippet causes memory access after free:

void test_transform_vector() {
    std::vector<MyMoveOnly> vec{};
    vec.emplace_back(MyMoveOnly(41));
    vec.emplace_back(MyMoveOnly(42));
    vec.emplace_back(MyMoveOnly(43));

    auto transformed1 = iter::imap([](MyMoveOnly& el) {
        el.set_val(el.get_val() + 10);
        return std::move(el);
    }, vec);
    auto transformed2 = iter::imap([](MyMoveOnly& el) -> MyMoveOnly& {
        el.set_val(el.get_val() - 10);
        // this should not compile, 
        // returning temporary (created by
        // previous lambda) by reference here
        return el;
    }, transformed1);
    auto filtered = iter::filter([](MyMoveOnly& el) {
        return 42 != el.get_val();
    }, transformed2);

    for (auto&& el : filtered) {
        std::cout << el.get_val() << std::endl;
    }
}

from cppitertools.

You are right. It seems to be a forwarding issue. I've got it fixed in the cpp14 branch, I'm working on the c++11 version atm.

from cppitertools.

Alright got it. This example produces a compile time error. And the best part is the syntax is beautiful

from cppitertools.

Great, thanks, all examples I tried work fine now, I am closing this.

from cppitertools.

Cool. Thank you for exposing this, I hadn't really considered examples like
that before
On Feb 18, 2015 2:44 PM, "staticlibs" [email protected] wrote:

Great, thanks, all examples I tried work fine now, I am closing this.

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

from cppitertools.