Database2Values, argument conversion implemented

[Aaron, 2008-05-11 18:11:20]

http://codereview.appspot.com/717/diff/1/6
File gears/database2/statement.cc (right):

http://codereview.appspot.com/717/diff/1/6#newcode75
Line 75: if (sql_arguments.token() == NULL) {
It's not a good idea to compare token() to NULL. I don't think there's any
guarantee that the js engine represents null js values using NULL.

We have a standard way to test for optional arguments in the Gears APIs, using
GetArguments(). I would recommend using that, and then passing a pointer to
JsArray instead of a reference. If the user did not supply arguments, pass a
NULL pointer. Then you can compare that pointer to NULL.

http://codereview.appspot.com/717/diff/1/6#newcode85
Line 85: // returning with failure will trigger an internal error
Silent failures look weird to me. Maybe assert(false) here.

http://codereview.appspot.com/717/diff/1/6#newcode111
Line 111: JsParamToSend *param = arguments + index;
Does arguments[index] work? I think it's easier to understand.

http://codereview.appspot.com/717/diff/1/6#newcode116
Line 116: value.reset(new int);
The heap allocated integers are unfortuante :-/. I guess this is why variants in
all the scripting languages are implemented as unions.

I think that long term we want to implement our own Variant, implemented as a
union of bool, int, int64*, string*, JsObject*, JsArray*, vector<Variant>*,
map<string, Variant>*. Then we can replace JsParamToSend with this class and
refactor the MarshaledJsToken code to use it. Then this would all come for free.

But I thought about it, and imho, moving forward with Database2 is more
important than gettng sidetracked on the variant thing, so this is fine for now.
Maybe just add a TODO in the definition of this class.

http://codereview.appspot.com/717/diff/1/7
File gears/database2/statement.h (right):

http://codereview.appspot.com/717/diff/1/7#newcode96
Line 96: int length_;
Should length be passed separately? The way it is here, the length value will be
duplicated in every row of a database result because the column length is always
the same.

The downside is you'd have to take out the assert()s in the .GetAs*() methods,
but I think you could move those asserts to the call sites maybe.

[Dimitri, 2008-05-13 22:52:02]

ready for another look.

http://codereview.appspot.com/717/diff/1/6
File gears/database2/statement.cc (right):

http://codereview.appspot.com/717/diff/1/6#newcode75
Line 75: if (sql_arguments.token() == NULL) {
On 2008/05/11 18:11:21, Aaron wrote:
> We have a standard way to test for optional arguments in the Gears APIs, using
> GetArguments(). I would recommend using that, and then passing a pointer to
> JsArray instead of a reference. If the user did not supply arguments, pass a
> NULL pointer. Then you can compare that pointer to NULL.

Thank you for making me look this up, cleared up for a lot of things me. As a
result, I also reworked Database2Statement::Create method and the way statement
callbacks are checked for existence.

http://codereview.appspot.com/717/diff/1/6#newcode85
Line 85: // returning with failure will trigger an internal error
On 2008/05/11 18:11:21, Aaron wrote:
> Silent failures look weird to me. Maybe assert(false) here.
 
Done.

http://codereview.appspot.com/717/diff/1/6#newcode111
Line 111: JsParamToSend *param = arguments + index;
On 2008/05/11 18:11:21, Aaron wrote:
> Does arguments[index] work? I think it's easier to understand.

Easified.

http://codereview.appspot.com/717/diff/1/6#newcode116
Line 116: value.reset(new int);
On 2008/05/11 18:11:21, Aaron wrote:
> The heap allocated integers are unfortuante :-/. I guess this is why variants
in
> all the scripting languages are implemented as unions.

Yes, I agree. 

> I think that long term we want to implement our own Variant, implemented as a
> union of bool, int, int64*, string*, JsObject*, JsArray*, vector<variant>*,
> map<string, variant="">*. Then we can replace JsParamToSend with this class
and
> refactor the MarshaledJsToken code to use it. Then this would all come for
free.

Yipee!

> But I thought about it, and imho, moving forward with Database2 is more
> important than gettng sidetracked on the variant thing, so this is fine for
now.
> Maybe just add a TODO in the definition of this class.

Done.

http://codereview.appspot.com/717/diff/1/7
File gears/database2/statement.h (right):

http://codereview.appspot.com/717/diff/1/7#newcode96
Line 96: int length_;
On 2008/05/11 18:11:21, Aaron wrote:
> Should length be passed separately? The way it is here, the length value will
be
> duplicated in every row of a database result because the column length is
always
> the same.
> 
> The downside is you'd have to take out the assert()s in the .GetAs*() methods,
> but I think you could move those asserts to the call sites maybe.

as discussed over IM, leaving length_ in because it's needed by destructor.

[Dimitri, 2008-05-22 12:12:24]

Per our discussion, I added Variant struct and multiple-row capability to
Database2Values.

[Aaron, 2008-05-22 22:16:41]

LGTM

http://codereview.appspot.com/717/diff/181/206
File gears/database2/statement.h (right):

http://codereview.appspot.com/717/diff/181/206#newcode78
Line 78: class Database2Values {
The shape of this internal class is really bugging me. It seems like it really
wants to just be a vector of vectors of Variant. 

But in the interest of moving forward, can you just slap a "TODO: clean me up"
on it?

[Dimitri, 2008-05-24 02:13:40]

the code is now checked into databas2 branch.

http://codereview.appspot.com/717/diff/181/206
File gears/database2/statement.h (right):

http://codereview.appspot.com/717/diff/181/206#newcode78
Line 78: class Database2Values {
On 2008/05/22 22:16:42, Aaron wrote:
> The shape of this internal class is really bugging me. It seems like it really
> wants to just be a vector of vectors of Variant. 
> 
> But in the interest of moving forward, can you just slap a "TODO: clean me up"
> on it?

Done.