[patches] Re: [PATCH v2 2/2] sql: use Tarantool routine to get collation

[n.pettik]

> On 10 Mar 2018, at 14:51, v.shpilevoy at tarantool.org wrote:
> 
> See below 3 comments.
> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 9f45c6224..ca4291bc3 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1165,6 +1167,73 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
>> 	}
>> }
>> 
>> +/**
>> + * Return name of given column collation from table.
>> + *
>> + * @param table Table which is used to fetch column.
>> + * @param column Number of column.
>> + * @retval Pointer to collation's name.
>> + */
>> +char *
> 
> 1. How about to return const char * instead of char *? It would be nice if compiler checks for us that this name is not modified or destroyed by free().

Done:

-char *
+const char *
 column_collation_name(Table *table, uint32_t column)


> 
>> +column_collation_name(Table *table, uint32_t column)
>> +{
>> +	assert(table != NULL);
>> 
>> +
>> +/**
>> + * Return name of given column collation from index.
>> + *
>> + * @param idx Index which is used to fetch column.
>> + * @param column Number of column.
>> + * @retval Pointer to collation's name.
>> + */
>> +char *
> 
> 2. Same as 1.

Done:

-char *
+const char *
 index_collation_name(Index *idx, uint32_t column)

> 
>> +index_collation_name(Index *idx, uint32_t column)
>> +{
>> +	assert(idx != NULL);
>> 
>> diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
>> index 5f2ddeaee..270265725 100644
>> --- a/src/box/sql/callback.c
>> +++ b/src/box/sql/callback.c
>> @@ -60,11 +60,12 @@ sqlite3GetCollSeq(Parse * pParse,	/* Parsing context */
>> 		  const char *zName	/* Collating sequence name */
>>    )
>> {
>> +	(void)db;
> 
> 3. Why do not remove this argument?

Done:

 struct coll *
 sqlite3GetCollSeq(Parse * pParse,      /* Parsing context */
-                 sqlite3 * db, /* if called during runtime - pointer to the DB */
                  struct coll * pColl,  /* Collating sequence with native encoding, or NULL */
                  const char *zName     /* Collating sequence name */
     )
 {
-       (void)db;