Add pass-by-reference option for operator>>

[faultyserver]

While trying to make good use of the lambda syntax for pulling out results from a query, I hit an issue where the function is being passed by value (and thus copied), causing my results to get lost when the operator exited and the variable went out of scope.

In short, I want to create class instances for each result row, but also want to avoid code repetition. I currently have something similar to this:

// Note that this example is stripped down, and missing a lot of boilerplate.
class Route;

template<typename Target, typename... AttrTypes>
class Builder {
  public:
    std::vector<Target> results;

    void operator()(AttrTypes... args) {
      results.emplace_back(std::forward<AttrTypes&&>(args)...);
    };
};

Builder<Route, std::string, std::string> route_builder;


class Route {
  public:
    std::string attribute1;
    std::string attribute2;

    Route(std::string a1, std::string a2) : attribute1(a1), attribute2(a2) {};

    std::vector<Route> all(sqlite::database db) {
      route_builder.results.empty();
      db << "SELECT * FROM routes;"
        >> route_builder;
      return route_builder.results;
    };
};

The relevant code for this issue is in Route::all() at the end of the code block above, with the expression >> route_builder. I am passing an instance of the Builder class, which implements operator() (as required by utility::function_traits), and is thus runnable by the >> implementation.

However, the current implementation takes the argument by value, meaning a local copy of the instance is created for use in the function, and any side effects are discarded when that variable falls out of scope (when operator>> returns).

In other words, route_builder.results - which I would expect to contain the Route instances of each row returned from the query - is actually empty, because the operations were performed on an ephemeral copy of the instance.

---

My solution to this was to modify the Function specialization of operator>> to take its parameter by reference:

template <typename Function>
typename std::enable_if<!is_sqlite_value<Function>::value, void>::type operator>>(
  Function& func) {
  typedef utility::function_traits<Function> traits;

  this->_extract([&func, this]() {
    binder<traits::arity>::run(*this, func);
  });
}

However, this does not work with the original, intended behavior of using a temporary lambda as the argument:

db << "SELECT route_short_name FROM route;" >> [&](std::string short_name) {
  // This fails to compile, since this lambda is not an lvalue and therefore
  // db::operator>> can not take a reference to it.
};

To get around this, a new specialization that takes an rvalue reference for these temporaries can be introduced. The resulting changes to the header would look like this:

template <typename Function>
typename std::enable_if<!is_sqlite_value<Function>::value, void>::type operator>>(
  Function& func) {
  ...
}

template <typename Function>
typename std::enable_if<!is_sqlite_value<Function>::value, void>::type operator>>(
  Function&& func) {
  ...
}

This works because lvalues will prefer matching to the lvalue reference specialization (the first one), and rvalue temporaries will prefer matching to the rvalue reference specialization.

I have tested this implementation with classes defining operator() and direct lambdas, and both are working as of now. That said, though, I do not consider myself an expert in C++, so I'm not sure if this is a feasible solution.

[bjoernpollex]

This seems to be the exact use-case for which reference-wrappers were designed. Would you consider that an option?

Basically, your all function would look like this:

std::vector<Route> all(sqlite::database db) {
  route_builder.results.empty();
  db << "SELECT * FROM routes;"
    >> std::ref(route_builder);
  return route_builder.results;
};

Note that route_builder is wrapped in a call to std::ref.

[faultyserver]

From the example in the docs on std::ref, it looks like std::ref is primarily intended for creating reference types to be passed to functions which take reference parameters (That's a terrible explanation. Hopefully the example they give makes it a little more clear). In this case, however, I'm trying to pass a reference to a function that takes a value, so I don't think std::ref will work as intended.

When I tried using std::ref as you showed above, I got this error:

include/modern_sqlite/utility/function_traits.h:12:13:
error: reference to overloaded function could not be resolved; did you mean to call it?
    decltype(&Function::operator())
             ^~~~~~~~~~~~~~~~~~~~~

which confuses me, because there are no obvious (to me) overloads of operator() anywhere in my code, or in std::reference_wrapper. Note that without std::ref, the code compiles/works, the only issue being the copy semantics invoked by the pass-by-value function signature.

That said, is there any advantage to using a pass-by-value signature and not implementing the pass-by-reference method? My thoughts on the matter:

• Passing by value doesn't readily allow using side-effects of the passed functions (the issue I originally ran into with the empty result vector).
• With C++11, rvalues (like temporary lambdas) can be passed "by reference" using the rvalue reference syntax - T&& - as I showed above.
• Using references avoids invoking unnecessary copy semantics. This is pretty minimal for simple types like lambdas. But with complex custom types (like my actual Builder class), invoking the copy constructor can be quite costly.

[Killili]

I see the point you are trying to make, and its a valid point.
But in this case isn't it a failure of the builder class for not having a working copy constructor? Or not being a proper Singleton? Even adding a copy constructor that just returns the object to be copied would solve this without the C++ Magic needed to work around it.

I'm trying to come up with better example where we would need that additional template but all i can think of would be solved by adding proper code or trivial in terms of performance loss.

Or and that's very possible i totally miss the point of this ;)

[aminroosta]

@faultyserver Thanks for the detailed explanation.

@bjoernpollex @Killili
I think faultyserver has a good point, we should support lvalue functors as well.
I did implement it in #61 vai Universal References, Let me know if you think something is missing.
If it looks good to you i will merge it tomorrow. (I think github recently added the git-apporve feature, i'm not sure if it is visible to you or not).

[aminroosta]

@Killili I also removed the extensions folder, since the type system is not working and those are extra files.
Sorry about that :-)

[faultyserver]

@aminroosta Wonderful! Your implementation certainly looks cleaner than mine. I was unaware of Universal References.

@Killili I think you were mostly on point, and a proper Singleton definitely would've solved the immediate issue as well. Thanks for the suggestion.

PS: Thanks for this line.

return std::move(person_builder.results); // move to avoid copying data ;-)