Alternative iterator interface #126

zauguin · 2017-05-24T08:46:29Z

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 · 2017-05-24T09:59:32Z

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 · 2018-01-21T20:14:25Z

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 · 2018-01-22T10:20:51Z

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 · 2018-01-23T11:36:48Z

hdr/sqlite_modern_cpp.h

+			}
+			template<class ...Types>
+			operator std::tuple<Types...>() {
+				std::tuple<Types...> value;


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

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.

I greed :-) 👍

aminroosta · 2018-01-23T11:45:54Z

hdr/sqlite_modern_cpp.h

-				tuple_iterate<std::tuple<Types...>>::iterate(values, *this);
-			});
+		// Not well-defined
+		row_iterator operator++(int);


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

@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".

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 · 2018-01-23T11:47:42Z

hdr/sqlite_modern_cpp.h

-			typedef utility::function_traits<Function> traits;
+	private:
+		database_binder *_binder = nullptr;
+		mutable value_type value{_binder}; // mutable, because `changing` the value is just reading it


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

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:

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.

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.

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.

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.

@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++.

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

@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.

@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 · 2018-01-23T11:55:52Z

@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 · 2018-01-23T14:54:08Z

@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 · 2018-01-23T18:18:36Z

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

aminroosta · 2018-01-24T02:22:59Z

@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 · 2018-01-24T02:27:05Z

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

aminroosta · 2018-02-04T02:36:06Z

@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 · 2018-02-05T14:36:41Z

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

aminroosta · 2018-02-05T17:32:17Z

@Killili thanks, I deleted all of them.

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

Alternative iterator interface

zauguin force-pushed the iterator3 branch from 37061ca to 69d7617 Compare May 24, 2017 14:42

zauguin force-pushed the iterator3 branch from 69d7617 to 8906d99 Compare July 13, 2017 11:54

zauguin added 8 commits January 21, 2018 20:49

Range interface

d35b6f0

Reimplement execute primitives with iterators

3d7e040

Fix bugs

a5f9f6e

Implement all executions with iterators

5bbe246

Less implicit conversations

30f9455

Remove trailing whitespace

4b765b1

Implement tuple extraction via operator>>

e761452

Added example to README

fde544f

zauguin force-pushed the iterator3 branch from e7b18f9 to fde544f Compare January 21, 2018 20:02

zauguin changed the title ~~[RFC] Alternative iterator interface~~ Alternative iterator interface Jan 21, 2018

Fix bug with experimental optional

8bc7d25

aminroosta requested changes Jan 23, 2018

View reviewed changes

Drop declaration of missing operator

73b062d

aminroosta mentioned this pull request Feb 4, 2018

dev branch: cmake + catch + travis fix #145

Merged

zauguin changed the base branch from master to dev February 4, 2018 10:57

Make execute inline

65544ca

zauguin added a commit to zauguin/sqlite_modern_cpp that referenced this pull request Feb 5, 2018

Merge pull request SqliteModernCpp#126 from zauguin/iterator3

8388219

Alternative iterator interface

zauguin force-pushed the iterator3 branch from 8388219 to 65544ca Compare February 5, 2018 18:46

zauguin merged commit 65544ca into SqliteModernCpp:dev Feb 5, 2018

zauguin mentioned this pull request Feb 7, 2018

[RFC] Iterator interface #103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative iterator interface #126

Alternative iterator interface #126

zauguin commented May 24, 2017

Killili commented May 24, 2017

zauguin commented Jan 21, 2018

Killili commented Jan 22, 2018

aminroosta Jan 23, 2018

zauguin Jan 23, 2018

aminroosta Jan 23, 2018

aminroosta Jan 23, 2018

zauguin Jan 23, 2018

aminroosta Jan 23, 2018

aminroosta Jan 23, 2018 •

edited

Loading

zauguin Jan 23, 2018

aminroosta Jan 23, 2018

aminroosta Jan 23, 2018

zauguin Jan 23, 2018

zauguin Feb 5, 2018

aminroosta commented Jan 23, 2018 •

edited

Loading

aminroosta commented Jan 23, 2018

zauguin commented Jan 23, 2018

aminroosta commented Jan 24, 2018 •

edited

Loading

aminroosta commented Jan 24, 2018

aminroosta commented Feb 4, 2018

Killili commented Feb 5, 2018

aminroosta commented Feb 5, 2018

Alternative iterator interface #126

Alternative iterator interface #126

Conversation

zauguin commented May 24, 2017

Killili commented May 24, 2017

zauguin commented Jan 21, 2018

Killili commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminroosta Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminroosta commented Jan 23, 2018 • edited Loading

aminroosta commented Jan 23, 2018

zauguin commented Jan 23, 2018

aminroosta commented Jan 24, 2018 • edited Loading

aminroosta commented Jan 24, 2018

aminroosta commented Feb 4, 2018

Killili commented Feb 5, 2018

aminroosta commented Feb 5, 2018

aminroosta Jan 23, 2018 •

edited

Loading

aminroosta commented Jan 23, 2018 •

edited

Loading

aminroosta commented Jan 24, 2018 •

edited

Loading