Soci 1057 #1084
Soci 1057 #1084
Conversation
There was a problem hiding this comment.
Sorry, I don't think this is correct. I'd also rather avoid changing the existing rowset_iterator ctor, we already have a ctor for creating an invalid iterator, so we should just use it instead.
Also, could you please rebase your branch on SOCI master to avoid having unrelated changes in this PR?
TIA!
| @@ -136,7 +141,7 @@ class rowset_impl | |||
| iterator begin() const | |||
| { | |||
| // No ownership transfer occurs here | |||
| return iterator(*st_, *define_); | |||
| return iterator(st_.get(), define_.get()); | |||
There was a problem hiding this comment.
I don't understand how does this not result in a crash... rowset_iterator ctor calls ++ which uses the pointer, which is null here for an invalid rowset.
Am I missing something here or would it actually crash if you added a test exercising the new code?
There was a problem hiding this comment.
I have been testing this and it does work. However, I also wonder why the ++(*this) doesn't make it explode... I can do a gdb session on it and see where it takes me.
Talking about the design itself, I agree that changing the rowset_iterator ctor is not ideal, however, I was taken away by the beauty of the minimal change to begin(). I did another version in which I didn't change the rowset_iterator instead I had:
iterator begin() const
{
// No ownership transfer occurs here
return (st_.get() == nullptr)
? iterator()
: iterator(*st_, *define_);
}
There was a problem hiding this comment.
Well, beauty is in the eye of the beholder and personally I find a ctor taking optional and maybe null pointers pretty ugly. And
iterator begin() const
{
// Empty rowset doesn't have any valid begin iterator.
return st_ ? iterator(*st_, *define_) : iterator();
}seems perfectly fine to me.
There was a problem hiding this comment.
It's coming up - I just need to test it - while I'm at it, should I add a clear for #198 and close a 10 year old issue? ;)
There was a problem hiding this comment.
Sure, adding clear() would be welcome as well. Just please add a unit test for it too. TIA!
This PR allows the creation of an empty rowset as per #1057.