Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(111)

Issue 924: sqlite3 pragma get/set user_version implemented (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 4 weeks ago by Dimitri
Modified:
3 months, 2 weeks ago
Reviewers:
Aaron, google-gears-eng, Scott.Hess
CC:
SVN Base:
http://google-gears.googlecode.com/svn/contrib/dimitri.glazkov/database2/gears/

Description

Added direct pragma modification calls to sqlite3 API.

Patch Set 1

Total comments: 23

Patch Set 2 : moved code to btree.c, restructured

Total comments: 9

Patch Set 3 : Simplified by removing transaction-creating, just report misuse now

Patch Set 4 : Removed starting a transaction, just report SQLITE_MISUSE now.

Total comments: 16
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
third_party/sqlite_google/preprocessed/sqlite3.h View 1 2 1 chunk 38 lines 0 comments Download
third_party/sqlite_google/src/btree.c View 2 2 chunks 50 lines 16 comments Download
third_party/sqlite_google/src/pragma.c View 1 2 2 chunks 51 lines 0 comments Download
third_party/sqlite_google/src/sqlite.h.in View 1 2 1 chunk 38 lines 0 comments Download

Messages

Total messages: 8
GvR
Dmitri, through scanning the logs for errors, I noticed the base url of your issue ...
6 months, 4 weeks ago
Aaron
LGTM
6 months, 4 weeks ago
Scott.Hess
Sorry for the delay - I'm gradually working my way through all my deferred gears-related ...
6 months, 2 weeks ago
Dimitri
Thanks for the comments, Scott! Ready for another look. http://codereview.appspot.com/924/diff/1/2 File third_party/sqlite_google/preprocessed/sqlite3.h (right): http://codereview.appspot.com/924/diff/1/2#newcode2689 Line ...
6 months, 2 weeks ago
Scott.Hess
Main thing about using the automated process for generating sqlite3.h is so that we don't ...
6 months, 2 weeks ago
Dimitri
Ready for another look. I'll add unit tests after rebranching. http://codereview.appspot.com/924/diff/83/141 File third_party/sqlite_google/src/btree.c (right): http://codereview.appspot.com/924/diff/83/141#newcode6438 ...
6 months ago
Scott.Hess
Looking good, I'm happier with the condensed not-mucking-with-transactions-at-all version. Left with style comments, which is ...
6 months ago
Dimitri
5 months, 3 weeks ago
Prettified. Unfortunately, due to branching, I am unable to put the new code
side-by-side. Instead, it's in it's own little world:
http://codereview.appspot.com/2101

http://codereview.appspot.com/924/diff/321/262
File third_party/sqlite_google/src/btree.c (right):

http://codereview.appspot.com/924/diff/321/262#newcode6432
Line 6432: int sqlite3_get_user_version(sqlite3 *sqlite, int *user_version) {
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Did you copy 'sqlite3 *sqlite' from somewhere?  If not, suggest using 'sqlite3
> *db'.  At this level, db is a squishy concept, but I think that's the form
used
> in other code.  [Like I said, if you're copying it from elsewhere in the
SQLite
> codebase, then ignore me.]

Nope, that was all me. Changed to *db.

> 
> I'd usually expect user_version to be piUserVersion in SQLiteland, which I
> believe may be somewhere near Hungary.

Ah, yes, the Magyar notation, we meet again. Changed to piUserVersion and
iUserVersion, respectivelly.

http://codereview.appspot.com/924/diff/321/262#newcode6435
Line 6435: if( sqlite == NULL ) return SQLITE_ERROR;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> SQLite style is 'sqlite==NULL' rather than 'sqlite == NULL'.  Yes, this is the
> opposite of Gears style :-).

Fixed.

http://codereview.appspot.com/924/diff/321/262#newcode6443
Line 6443: */
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Suggest dropping the caveat about unlikely things.  It either needs a lock
going
> in, or it doesn't.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6444
Line 6444: return sqlite3BtreeGetMeta(bt, kUserCookie, (u32 *)user_version);
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Why the cast?  Alternate phrasing: Why isn't user_verion just u32 from the
> get-go?
> 

That's easy to fix. I made user version unsigned int.

http://codereview.appspot.com/924/diff/321/262#newcode6447
Line 6447: int sqlite3_set_user_version(sqlite3 *sqlite, int user_version) {
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Comparable comments to above on sqlite/db and user_version/iUserVersion.

Fixed.

http://codereview.appspot.com/924/diff/321/262#newcode6450
Line 6450: if( sqlite == NULL ) return SQLITE_ERROR;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> No spaces around ==.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6456
Line 6456: if( bt->inTrans != TRANS_WRITE ) return SQLITE_MISUSE;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> No spaces around !=.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6458
Line 6458: return sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Comparable comment as above around cast.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld r373