[Monotone-devel] comments on .sqlite3.binary branch

[Nathaniel Smith]

I can see you're making an effort to match our standard style more
closely, which I really appreciate :-).  I did notice a few
places that were off, though, so I'll mention them again:
  -- in argument lists, always put a space after a comma
  -- braces are GNU style.  Not:
    do
    { stepresult=sqlite3_step(stmt);
      I(stepresult==SQLITE_DONE || stepresult==SQLITE_ROW);
      if (stepresult==SQLITE_ROW) 
        dump_row_cb(dump, stmt);
    } while (stepresult==SQLITE_ROW);
  -- need spaces on each side of all the = and == in the above, too.
I fixed all the places that I found.  A bit boring, but I guess
someone has to do it.

In mainline made the logging in fetch() only happen if --debug was
passed, because profiling showed it taking something like 15% of
the time in an initial pull.  The merger applied these changes to one
of the copies of the fetch() function in the branch, but not the
other.  I've fixed this.

Are pack/unpack/sqlite3_unpack_fn/sqlite3_unbase64_fn now dead code
that can be removed?

The changes to dump_row_cb look wrong -- shouldn't we just be putting
the sqlite3_stmt into the dump_request structure, instead of rewriting
dump_table_cb to iterate by hand?  I haven't looked super-closely at
this, though.

I feel like the queryarg stuff isn't _quite_ the right API yet; it
should be possible to make it a bit cleaner.  This might be something
to defer, though, I will see how much work it would be.

(Does inheriting from std::string to add a public bool attribute seem
a little odd to anyone else?  Obviously it works, but I would have
used a struct { std::string; bool; } instead.)

Ditto on Tim's comments.

I think that's everything; the cert base64ing stuff is clearly
suboptimal but I can see how one might want to put off touching cert
stuff for now -- cert.cc is ooold code...

Something we should vaguely consider -- switching to 'deflate' instead
of 'gzip', since we're rewriting all the packed values anyway.  This
saves 18 bytes per cell.  'db info' tells me that in my db, it would
save about half a meg (1%).  Not that big a deal, but it might make
some code simpler (Matt knows about this).

Need to go to bed now; I'll try to get to more details on the vague
things above tomorrow... :-)

Thanks for taking the lead on this,
-- Nathaniel

-- 
"...these, like all words, have single, decontextualized meanings: everyone
knows what each of these words means, everyone knows what constitutes an
instance of each of their referents.  Language is fixed.  Meaning is
certain.  Santa Claus comes down the chimney at midnight on December 24."
  -- The Language War, Robin Lakoff