[tarantool-patches] Re: [PATCH 2/4] sql: remove unused error codes, use enum instead of defines

["n.pettik" <korablev@tarantool.org>]

Your subject doesn’t fit into 50 lines and consists from two parts.
Maybe better to move it to commit message?

2 minor comments:

> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> ---
> src/box/sql/build.c     |  4 ---
> src/box/sql/main.c      | 32 ------------------
> src/box/sql/os_unix.c   |  9 +-----
> src/box/sql/sqliteInt.h | 86 ++++++++++++++++++++++++++++---------------------
> src/box/sql/vdbe.c      |  1 -
> src/box/sql/vdbeInt.h   |  5 +--
> src/box/sql/vdbeapi.c   |  9 ------
> src/box/sql/vdbeaux.c   |  1 -
> 8 files changed, 52 insertions(+), 95 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5e3ed0f39..4510df206 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -105,10 +105,6 @@ sqlite3FinishCoding(Parse * pParse)
> 
> 				if (db->init.busy == 0)
> 					sqlite3VdbeChangeP5(v, 1);
> -
> -				VdbeComment((v, "usesStmtJournal=%d",
> -					     pParse->mayAbort
> -					     && pParse->isMultiWrite));
> 			}
> 
> 			/* Code constant expressions that where factored out of inner loops */
> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index b77348c21..8e898178a 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -800,9 +800,6 @@ sqlite3ErrName(int rc)
> 		case SQLITE_ERROR:
> 			zName = "SQLITE_ERROR";
> 			break;
> -		case SQLITE_INTERNAL:
> -			zName = "SQLITE_INTERNAL";
> -			break;
> 		case SQLITE_PERM:
> 			zName = "SQLITE_PERM";
> 			break;
> @@ -821,9 +818,6 @@ sqlite3ErrName(int rc)
> 		case SQLITE_NOMEM:
> 			zName = "SQLITE_NOMEM";
> 			break;
> -		case SQLITE_READONLY:
> -			zName = "SQLITE_READONLY";
> -			break;
> 		case SQLITE_INTERRUPT:
> 			zName = "SQLITE_INTERRUPT";
> 			break;
> @@ -917,12 +911,6 @@ sqlite3ErrName(int rc)
> 		case SQLITE_CANTOPEN:
> 			zName = "SQLITE_CANTOPEN";
> 			break;
> -		case SQLITE_PROTOCOL:
> -			zName = "SQLITE_PROTOCOL";
> -			break;
> -		case SQLITE_EMPTY:
> -			zName = "SQLITE_EMPTY";
> -			break;
> 		case SQLITE_SCHEMA:
> 			zName = "SQLITE_SCHEMA";
> 			break;
> @@ -959,27 +947,15 @@ sqlite3ErrName(int rc)
> 		case SQLITE_MISUSE:
> 			zName = "SQLITE_MISUSE";
> 			break;
> -		case SQLITE_NOLFS:
> -			zName = "SQLITE_NOLFS";
> -			break;
> -		case SQLITE_FORMAT:
> -			zName = "SQLITE_FORMAT";
> -			break;
> 		case SQLITE_RANGE:
> 			zName = "SQLITE_RANGE";
> 			break;
> -		case SQLITE_NOTADB:
> -			zName = "SQLITE_NOTADB";
> -			break;
> 		case SQL_TARANTOOL_ERROR:
> 			zName = "SQLITE_TARANTOOL_ERROR";
> 			break;
> 		case SQLITE_ROW:
> 			zName = "SQLITE_ROW";
> 			break;
> -		case SQLITE_NOTICE:
> -			zName = "SQLITE_NOTICE";
> -			break;
> 		case SQLITE_WARNING:
> 			zName = "SQLITE_WARNING";
> 			break;
> @@ -1008,32 +984,24 @@ sqlite3ErrStr(int rc)
> 	static const char *const aMsg[] = {
> 		/* SQLITE_OK          */ "not an error",
> 		/* SQLITE_ERROR       */ "SQL logic error or missing database",
> -		/* SQLITE_INTERNAL    */ 0,
> 		/* SQLITE_PERM        */ "access permission denied",
> 		/* SQLITE_ABORT       */ "callback requested query abort",
> 		/* SQLITE_BUSY        */ "database is locked",
> 		/* SQLITE_LOCKED      */ "database table is locked",
> 		/* SQLITE_NOMEM       */ "out of memory",
> -		/* SQLITE_READONLY    */ "attempt to write a readonly database",
> 		/* SQLITE_INTERRUPT   */ "interrupted",
> 		/* SQLITE_IOERR       */ "disk I/O error",
> 		/* SQLITE_CORRUPT     */ "database disk image is malformed",
> 		/* SQLITE_NOTFOUND    */ "unknown operation",
> 		/* SQLITE_FULL        */ "database or disk is full",
> 		/* SQLITE_CANTOPEN    */ "unable to open database file",
> -		/* SQLITE_PROTOCOL    */ "locking protocol",
> -		/* SQLITE_EMPTY       */ "table contains no data",
> 		/* SQLITE_SCHEMA      */ "database schema has changed",
> 		/* SQLITE_TOOBIG      */ "string or blob too big",
> 		/* SQLITE_CONSTRAINT  */ "constraint failed",
> 		/* SQLITE_MISMATCH    */ "datatype mismatch",
> 		/* SQLITE_MISUSE      */
> 		    "library routine called out of sequence",
> -		/* SQLITE_NOLFS       */ "large file support is disabled",
> -		/* SQLITE_FORMAT      */ "auxiliary database format error",
> 		/* SQLITE_RANGE       */ "bind or column index out of range",
> -		/* SQLITE_NOTADB      */
> -		    "file is encrypted or is not a database",
> 		/* SQL_TARANTOOL_ITERATOR_FAIL */ "Tarantool's iterator failed",
> 		/* SQL_TARANTOOL_INSERT_FAIL */ "Tarantool's insert failed",
> 		/* SQL_TARANTOOL_DELETE_FAIL */ "Tarantool's delete failed",
> diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
> index 8dce6f0c2..9cb8a54d0 100644
> --- a/src/box/sql/os_unix.c
> +++ b/src/box/sql/os_unix.c
> @@ -1011,10 +1011,6 @@ findInodeInfo(unixFile * pFile,	/* Unix file with file desc used in the key */
> 	rc = osFstat(fd, &statbuf);
> 	if (rc != 0) {
> 		storeLastErrno(pFile, errno);
> -#if defined(EOVERFLOW) && defined(SQLITE_DISABLE_LFS)
> -		if (pFile->lastErrno == EOVERFLOW)
> -			return SQLITE_NOLFS;
> -#endif
> 		return SQLITE_IOERR;
> 	}
> #ifdef __APPLE__
> @@ -6011,10 +6007,7 @@ proxyFileControl(sqlite3_file * id, int op, void *pArg)
> 			int isProxyStyle = (pFile->pMethod == &proxyIoMethods);
> 			if (pArg == NULL || (const char *)pArg == 0) {
> 				if (isProxyStyle) {
> -					/* turn off proxy locking - not supported. */
> -					rc = SQLITE_ERROR
> -					    /*SQLITE_PROTOCOL? SQLITE_MISUSE? */
> -					    ;
> +					rc = SQLITE_ERROR;
> 				} else {
> 					/* turn off proxy locking - already off - NOOP */
> 					rc = SQLITE_OK;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e786cf842..f78a93a71 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -405,42 +405,56 @@ struct sqlite3_vfs {
> #define SQLITE_LIMIT_TRIGGER_DEPTH             9
> #define SQLITE_LIMIT_WORKER_THREADS           10
> 
> -#define SQLITE_OK           0	/* Successful result */
> -/* beginning-of-error-codes */
> -#define SQLITE_ERROR        1	/* SQL error or missing database */
> -#define SQLITE_INTERNAL     2	/* Internal logic error in SQLite */
> -#define SQLITE_PERM         3	/* Access permission denied */
> -#define SQLITE_ABORT        4	/* Callback routine requested an abort */
> -#define SQLITE_BUSY         5	/* The database file is locked */
> -#define SQLITE_LOCKED       6	/* A table in the database is locked */
> -#define SQLITE_NOMEM        7	/* A malloc() failed */
> -#define SQLITE_READONLY     8	/* Attempt to write a readonly database */
> -#define SQLITE_INTERRUPT    9	/* Operation terminated by sqlite3_interrupt() */
> -#define SQLITE_IOERR       10	/* Some kind of disk I/O error occurred */
> -#define SQLITE_CORRUPT     11	/* The database disk image is malformed */
> -#define SQLITE_NOTFOUND    12	/* Unknown opcode in sqlite3_file_control() */
> -#define SQLITE_FULL        13	/* Insertion failed because database is full */
> -#define SQLITE_CANTOPEN    14	/* Unable to open the database file */
> -#define SQLITE_PROTOCOL    15	/* Database lock protocol error */
> -#define SQLITE_EMPTY       16	/* Database is empty */
> -#define SQLITE_SCHEMA      17	/* The database schema changed */
> -#define SQLITE_TOOBIG      18	/* String or BLOB exceeds size limit */
> -#define SQLITE_CONSTRAINT  19	/* Abort due to constraint violation */
> -#define SQLITE_MISMATCH    20	/* Data type mismatch */
> -#define SQLITE_MISUSE      21	/* Library used incorrectly */
> -#define SQLITE_NOLFS       22	/* Uses OS features not supported on host */
> -#define SQLITE_FORMAT      23	/* Auxiliary database format error */
> -#define SQLITE_RANGE       24	/* 2nd parameter to sqlite3_bind out of range */
> -#define SQLITE_NOTADB      25	/* File opened that is not a database file */
> -#define SQL_TARANTOOL_ITERATOR_FAIL 26
> -#define SQL_TARANTOOL_INSERT_FAIL   27
> -#define SQL_TARANTOOL_DELETE_FAIL   28
> -#define SQL_TARANTOOL_ERROR         29
> -#define SQLITE_NOTICE      31	/* Notifications from sqlite3_log() */
> -#define SQLITE_WARNING     32	/* Warnings from sqlite3_log() */
> -#define SQLITE_ROW         100	/* sqlite3_step() has another row ready */
> -#define SQLITE_DONE        101	/* sqlite3_step() has finished executing */
> -/* end-of-error-codes */
> +enum sql_ret_code {
> +	/** Result of a routine is ok. */
> +	SQLITE_OK = 0,
> +	/** Common error code. */
> +	SQLITE_ERROR,
> +	/** Access permission denied. */
> +	SQLITE_PERM,
> +	/** Callback routine requested an abort. */
> +	SQLITE_ABORT,
> +	/** The database file is locked. */
> +	SQLITE_BUSY,
> +	/** A table in the database is locked. */
> +	SQLITE_LOCKED,
> +	/** A malloc() failed. */
> +	SQLITE_NOMEM,
> +	/** Operation terminated by sqlite3_interrupt(). */
> +	SQLITE_INTERRUPT,
> +	/** Some kind of disk I/O error occurred. */
> +	SQLITE_IOERR,
> +	/** The database disk image is malformed. */
> +	SQLITE_CORRUPT,
> +	/** Unknown opcode in sqlite3_file_control(). */
> +	SQLITE_NOTFOUND,
> +	/** Insertion failed because database is full. */
> +	SQLITE_FULL,
> +	/** Unable to open the database file. */
> +	SQLITE_CANTOPEN,
> +	/** The database schema changed. */
> +	SQLITE_SCHEMA,
> +	/** String or BLOB exceeds size limit. */
> +	SQLITE_TOOBIG,
> +	/** Abort due to constraint violation. */
> +	SQLITE_CONSTRAINT,
> +	/** Data type mismatch. */
> +	SQLITE_MISMATCH,
> +	/** Library used incorrectly. */
> +	SQLITE_MISUSE,
> +	/** 2nd parameter to sqlite3_bind out of range. */
> +	SQLITE_RANGE,
> +	SQL_TARANTOOL_ITERATOR_FAIL,
> +	SQL_TARANTOOL_INSERT_FAIL,
> +	SQL_TARANTOOL_DELETE_FAIL,
> +	SQL_TARANTOOL_ERROR,
> +	/** Warnings from sqlite3_log(). */
> +	SQLITE_WARNING,
> +	/** sqlite3_step() has another row ready. */
> +	SQLITE_ROW,
> +	/** sqlite3_step() has finished executing. */
> +	SQLITE_DONE,
> +};

I do like move to enum, but what about removing this obsolete ’SQLITE’ prefix?
Let it be just ’SQL_’. Moreover, some error seems to be SQLite specific.
Great time to remove them, isn’t it?
If you don’t want or don’t have time for this boring routine, you can create issue
with ‘good first issue’ label.

> 
> void *
> sqlite3_malloc(int);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index bf8a27709..1685c27ce 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1419,7 +1419,6 @@ case OP_ResultRow: {
> 	 */
> 	if (SQLITE_OK!=(rc = sqlite3VdbeCheckFk(p, 0))) {
> 		assert(user_session->sql_flags&SQLITE_CountRows);
> -		assert(p->usesStmtJournal);
> 		goto abort_due_to_error;
> 	}
> 
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index fcb45c8a8..f8f4e566b 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -418,9 +418,6 @@ struct Vdbe {
> 	i64 startTime;		/* Time when query started - used for profiling */
> #endif
> 	int nOp;		/* Number of instructions in the program */
> -#ifdef SQLITE_DEBUG
> -	int rcApp;		/* errcode set by sqlite3_result_error_code() */
> -#endif
> 	u16 nResColumn;		/* Number of columns in one row of the result set */
> 	u8 errorAction;		/* Recovery action to do in case of an error */
> 	bft expired:1;		/* True if the VM needs to be recompiled */
> @@ -428,7 +425,6 @@ struct Vdbe {
> 	bft explain:2;		/* True if EXPLAIN present on SQL command */
> 	bft changeCntOn:1;	/* True to update the change-counter */
> 	bft runOnlyOnce:1;	/* Automatically expire on reset */
> -	bft usesStmtJournal:1;	/* True if uses a statement journal */
> 	bft isPrepareV2:1;	/* True if prepared with prepare_v2() */
> 	u32 aCounter[5];	/* Counters used by sqlite3_stmt_status() */
> 	char *zSql;		/* Text of the SQL statement that generated this */
> @@ -446,6 +442,7 @@ struct Vdbe {
> 	int nScan;		/* Entries in aScan[] */
> 	ScanStatus *aScan;	/* Scan definitions for sqlite3_stmt_scanstatus() */
> #endif
> +

Nitpicking: extra diff.

> };
> 
> /*
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index dff1e1697..4c6e90c64 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -469,10 +469,6 @@ sqlite3_result_error_code(sqlite3_context * pCtx, int errCode)
> {
> 	pCtx->isError = errCode;
> 	pCtx->fErrorOrAux = 1;
> -#ifdef SQLITE_DEBUG
> -	if (pCtx->pVdbe)
> -		pCtx->pVdbe->rcApp = errCode;
> -#endif
> 	if (pCtx->pOut->flags & MEM_Null) {
> 		sqlite3VdbeMemSetStr(pCtx->pOut, sqlite3ErrStr(errCode), -1, 1,
> 				     SQLITE_STATIC);
> @@ -580,9 +576,6 @@ sqlite3Step(Vdbe * p)
> 		db->nVdbeActive++;
> 		p->pc = 0;
> 	}
> -#ifdef SQLITE_DEBUG
> -	p->rcApp = SQLITE_OK;
> -#endif
> #ifndef SQLITE_OMIT_EXPLAIN
> 	if (p->explain) {
> 		rc = sqlite3VdbeList(p);
> @@ -614,8 +607,6 @@ sqlite3Step(Vdbe * p)
> 	 */
> 	assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
> 	       || (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
> -	assert((p->rc != SQLITE_ROW && p->rc != SQLITE_DONE)
> -	       || p->rc == p->rcApp);
> 	if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
> 		/* If this statement was prepared using sqlite3_prepare_v2(), and an
> 		 * error has occurred, then return the error code in p->rc to the
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index e4335524a..bb121a318 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2148,7 +2148,6 @@ sqlite3VdbeMakeReady(Vdbe * p,	/* The VDBE */
> 	assert(EIGHT_BYTE_ALIGNMENT(&x.pSpace[x.nFree]));
> 
> 	resolveP2Values(p, &nArg);
> -	p->usesStmtJournal = (u8) (pParse->isMultiWrite && pParse->mayAbort);
> 	if (pParse->explain && nMem < 10) {
> 		nMem = 10;
> 	}
> -- 
> 2.14.3 (Apple Git-98)
> 
>