Alternative iterator interface

[zauguin]

I think this feels more natural than the interface from #103 and it makes lifetime questions easier.
The database_binder behaves like a container now:

for(auto &&row : db << "select age,name,weight from user where age > ? ;" << 21) {
  int _age;
  string _name;
  double _weight;
  row >> _age >> _name >> _weight;
  cout << _age << ' ' << _name << ' ' << _weight << endl;
}
// or
for(std::tuple<int, string, double> row : db << "select age,name,weight from user where age > ? ;" << 21) {
  cout << std::get<int>(row) << ' ' << std::get<string>(row) << ' ' << std::get<double>(row) << endl;
}

@Killili What do you think? It's not our usual stream-op interface, but I think this makes it clearer that the range doesn't exists independent from the statement.

[Killili]

I like it more then #103 it looks more consistent with the rest of the API.

But now that i think about it, isnt it a lot like the prepared statement stuff?

auto prep = db << "select... where ?"; // create
prep << 21; // bind
auto itr = prep.begin() // "execute" // till here the prep statement does the same
auto row = itr++; // iterate

Maybe there is some synergy that could be used?

[zauguin]

I added some documentation and rebased it on the current master.
The old code for executing prepared statements is now internally implemented by using this interface.

If noone objects I would merge it soon.

[Killili]

I'm totaly out the loop here. Didnt touch C++ for the last 10+ Month. So this look like magic now ;)
So i can only rely on my past selfs comment up there and he liked it. So great work bringing it to this point. But someone else should take a look. @aminroosta

[aminroosta]

@zauguin This constructs an empty tuple<Types...> and then assigns to each tuple elements.
Can we avoid the overhead?

[zauguin]

The problem is that our interface always gets a referrence to a preexisting object.
So fixing this would require to rewrite get_col_from_db for every type to return the value instead of storing it in a parameter.
This is a streight forward conversion but I think it would be better to do this in a separate Pull Request because it would add another 100+ line of changes, so reviewing this together could easiliy become be a pain.

Also the overhead should be small or nonexistent: The values are default constructed, then in a probably inlined function a value is assigned, so most optimizers should eliminate the additional construction anyway.

[aminroosta]

I greed :-) 👍

[aminroosta]

@zauguin I think we should not declare the postfix operator, a compile error is better than a linker one.

[zauguin]

@aminroosta This was added to be compatible with the Ranges TS.
The operator++ will normally not be necessary, but it is checked for with concepts.
But this was a hack in the first place and I agree moving a compile-time to a link-time error is bad.

We can either just take it out. This is the nice way and we don't lie about our type. The downside would themissing compatibility with Ranges-v3 or the Ranges TS.

We could also rewrite it to fail at compile time iff we are in an evaluated context, so using it would lead to a compile-time error, but checking for it would show it's existance.
Obviously this would really be a ugly hack, but we could interact with most libraries expecting "real iterators".

[aminroosta]

We can either just take it out. This is the nice way and we don't lie about our type. The downside would themissing compatibility with Ranges-v3 or the Ranges TS.

I think we should do that, unless we can come up with so called "real iterators" :-)

[aminroosta]

@zauguin I'm just wondering since we have no const member function modifying the value, why do we need mark it as mutable?

[zauguin]

If this is not mutable, operator* has to return a const reference to value_type instead.
But reading from the value_type requires increasing the index for the next column, next_index.

I see four ways to solve this:

1. Make operator * and operator -> not const.
   This breaks requirements for iterators and might break code. IMO it would also be weird, because these functions are not changing anything.
2. Make reference the same as value_type and return a new value_type at every call to the operators.
   If this is ever used with a not range for loop, everytime *iter is used the read restarts at the first column.
   So this could lead to very hard to find bugs.
3. Make the value_type member functions const and value_type::next_index mutable. Then a const reference can be returned.
   This would work, but then we work const qualified instances of value_type, which don't act like const objects at all because they change if any function on them is called.
4. The current approach: A mutable value.
   This is certainly not perfect, but it moves the problems of 3. to the iterator type which is less visible to the user.

[aminroosta]

@zauguin Thanks for the clarification, Since value_type does not own the underlying data, I too think solution No 4 is the right choice here.

Right now value_type will only read data if dereferenced (*value) or when it is implicitly converted to std::tuple, basically a non-owing type.

What are your thoughts if we make it own the entire row?
value_type can own an std::tuple member, or drive from std::tuple<Types...>
We will initialize & update the underlying tuple in row_iterator::operator++.

[aminroosta]

@zauguin If we drive from an std::tuple, can we take advantage of C++17 structured bindings?

[zauguin]

@aminroosta This sounds like the interface of #103.
The fundamental problem is determining the type and number of columns at compile time. If the value_type should own the values, the value_type has to know about <Types...>, so the types have to be passed as explicit template arguments.

[zauguin]

@aminroosta How do you prefere to move forward here? Do you prefer the interface opions discussed in #103 or should we continue with this one?

[aminroosta]

@zauguin Thanks for the really great work, This PR looks really interesting.
Sorry for the late reply, I missed the github notification.

I've added a review, But I think it looks good to merge even in its current shape.

[aminroosta]

@zauguin A quick discussion about the branches we are using.

I will propose, we should have a dev branch,
This PR, also the PR I had for blob_t could go to that branch.
It will effectively be the branch for the next release.
We will mention it in the master readme.

This way can have an experimental(dev) & stable(master) branch.

[zauguin]

@aminroosta This sounds like a great plan. Will you create the branch and set everything up?

[aminroosta]

@zauguin Great, I will try to setup that by the end of this week.

I am planning to do the following:

• Replace autotools with Cmake & add Catch.
• Fix broken travis builds, happened after migrating to an organization.
• Remove boost support.
• Migrate our tests to use Catch.

[aminroosta]

Here is the WIP https://github.com/SqliteModernCpp/sqlite_modern_cpp/compare/master...aminroosta:dev?expand=1

[aminroosta]

@Killili
I have removed my old branches from the repo.
(except blob_t, which I will remove after merging #125).

There are 3 branches from you too (fix-138, nulltype, custom_db), could you please have a look.
Is it ok if we remove them? since we want to only keep master & dev from now on.

[Killili]

@aminroosta i'm not sure what these branches do, so they are probably not important.

[aminroosta]

@Killili thanks, I deleted all of them.

@zauguin There is a merge conflict, please resolve this so we can merge this PR.
Thanks.