Re: [OT] Re: Binding rvalues to const ref in D
On 10/20/16 5:07 PM, Nick Sabalausky wrote: I think I'm going to try to put out first-try pass at a new API in a separate branch, try to get that out as soon as I can, and post it for experimentation/feedback. Awesome! Looking forward to it. -Steve
Re: [OT] Re: Binding rvalues to const ref in D
On 10/20/2016 04:32 PM, Steven Schveighoffer wrote: On 10/20/16 12:50 PM, Nick Sabalausky wrote: You can't bind individual values? Is there something wrong with "bindParameter(value, paramIndex)"? (I mean, besides the fact that it takes a ref, and, like the rest of the lib, isn't really documented anywhere outside of the code itself.) [...examples involving out-of-scope data...] Now, there is bindParameters(Variant[]), which binds the *value* stored in each parameter to the fields. This was the only way I could do it without having to allocate space for individual values. But you must bind everything at once! Ok, I see. Right. Actually I hit the same problem myself yesterday adding a test for a PR that added support for setting null via Variant(null) instead of setNullParam. The bindParameters(Variant[]) was the only one I could use because you can't pass a null literal by ref. Honestly, the most egregious issue is the lifetime management. In some cases, if you pass or copy resource wrappers, the destructor will close the connection, or the above thing about having to allocate a place for values so you can bind parameters without worrying about their lifetimes going away. Wrapping mysql-native (which should be concerned mostly with low-level stuff) so I can make more suitable ranges out of the data was really hard, I ended up having to use RefCounted to make sure all the resource handles didn't go away! Right, gotcha. I hadn't really hit that much myself in the past because for a while I hadn't really been using the prepared statements much, nor using it without vibe's connection pool. But you're right, this stuff definitely needs fixed. I'll take a look when I can. One other thing API-wise that is horrendous is the handling of null parameters (especially when you have to insert multiple rows with the same prepared statement, and sometimes you have some fields that should be null). Nullable!T works awesome for vibe, I think mysql-native should use that model. Yea, nulls were kind of always an awkward thing in the lib. I think the lib's original design might predate Nullable, which, I too am a fan of. I think I'm going to try to put out first-try pass at a new API in a separate branch, try to get that out as soon as I can, and post it for experimentation/feedback.
Re: [OT] Re: Binding rvalues to const ref in D
On 10/20/16 12:50 PM, Nick Sabalausky wrote:
On 10/20/2016 09:33 AM, Steven Schveighoffer wrote:
Yes, it does work. However, one thing that I *sorely* miss is the
ability to simply bind an individual value.
At the moment, in order to bind a value, you have to pass an array of
Variant for all the values. I currently have a whole wrapper around this
library to make it more palatable, and to fix the lifetime issues.
You can't bind individual values? Is there something wrong with
"bindParameter(value, paramIndex)"? (I mean, besides the fact that it
takes a ref, and, like the rest of the lib, isn't really documented
anywhere outside of the code itself.)
Yes, because bindParameter(myint + 5, idx) doesn't work. And this is
even worse:
if(x == 5)
{
int y = 6;
cmd.bindParameter(y, 2);
} // oops, y is now gone!
Or maybe this:
foreach(i, j; someRange)
{
cmd.bindParameter(j, i);
}// now all are bound to reference the same non-existent memory
In order for Command struct to legitimately keep references to arbitrary
value types, you need to put the storage somewhere. This isn't very
conducive to how D programs are written.
Now, there is bindParameters(Variant[]), which binds the *value* stored
in each parameter to the fields. This was the only way I could do it
without having to allocate space for individual values. But you must
bind everything at once!
I do agree though, mysql-native *definitely* needs an API refresh. (In
fact, I just happened to post several issues regarding that yesterday,
and another person posted one as well. I want to take care of this ASAP,
especially b/c it makes sense to do so before fixing the near-total lack
of docs, which is already in desperate need of addressing.)
Since you've found the need to wrap the API, would you mind taking a
look through the current list of issues I've tagged "api" (although I
see several of them are yours), and post any thoughts or add any
additional issues you might have? I'd like to address these things ASAP,
and input from people who use the lib and have issues with the API would
be highly valuable:
https://github.com/mysql-d/mysql-native/issues
Honestly, the most egregious issue is the lifetime management. In some
cases, if you pass or copy resource wrappers, the destructor will close
the connection, or the above thing about having to allocate a place for
values so you can bind parameters without worrying about their lifetimes
going away. Wrapping mysql-native (which should be concerned mostly with
low-level stuff) so I can make more suitable ranges out of the data was
really hard, I ended up having to use RefCounted to make sure all the
resource handles didn't go away!
I'll take a look when I can. One other thing API-wise that is horrendous
is the handling of null parameters (especially when you have to insert
multiple rows with the same prepared statement, and sometimes you have
some fields that should be null). Nullable!T works awesome for vibe, I
think mysql-native should use that model.
-Steve
Re: [OT] Re: Binding rvalues to const ref in D
On 10/20/2016 09:33 AM, Steven Schveighoffer wrote: Yes, it does work. However, one thing that I *sorely* miss is the ability to simply bind an individual value. At the moment, in order to bind a value, you have to pass an array of Variant for all the values. I currently have a whole wrapper around this library to make it more palatable, and to fix the lifetime issues. You can't bind individual values? Is there something wrong with "bindParameter(value, paramIndex)"? (I mean, besides the fact that it takes a ref, and, like the rest of the lib, isn't really documented anywhere outside of the code itself.) I do agree though, mysql-native *definitely* needs an API refresh. (In fact, I just happened to post several issues regarding that yesterday, and another person posted one as well. I want to take care of this ASAP, especially b/c it makes sense to do so before fixing the near-total lack of docs, which is already in desperate need of addressing.) Since you've found the need to wrap the API, would you mind taking a look through the current list of issues I've tagged "api" (although I see several of them are yours), and post any thoughts or add any additional issues you might have? I'd like to address these things ASAP, and input from people who use the lib and have issues with the API would be highly valuable: https://github.com/mysql-d/mysql-native/issues
Re: [OT] Re: Binding rvalues to const ref in D
On 10/20/16 2:38 AM, Nick Sabalausky wrote: On 10/19/2016 07:04 PM, Chris Wright wrote: Right. For instance, binding query parameters with mysql-native. The thing you're binding is passed by reference and I'm not sure why. It's been like that since mysql-native's original release, by the original author, some years ago. I suspect the idea was a rudimentary ORM-like approach: to have the prepared statement params semi-permanently tied to actual variables (ie, "bound" to them). Ie, so you could re-exectute the same prepared statement with different values just by changing the values and calling `execPrepared` again, without calling any of the bind functions again. I'd have to check whether or not that usage pattern currently works though. Yes, it does work. However, one thing that I *sorely* miss is the ability to simply bind an individual value. At the moment, in order to bind a value, you have to pass an array of Variant for all the values. I currently have a whole wrapper around this library to make it more palatable, and to fix the lifetime issues. -Steve
