[vlc-devel] [PATCH] Prepared statements for SQL

Laurent Aimar fenrir at via.ecp.fr
Tue Oct 20 19:18:03 CEST 2009


Hi,

> +#define VLC_SQL_ROW 1
> +#define VLC_SQL_DONE 2
 What are they used for? A doxy comment would be usefull.

> +typedef enum {
> +    SQL_INT,
> +    SQL_DOUBLE,
> +    SQL_TEXT,
> +    SQL_BLOB,
> +    SQL_NULL
> +} sql_type_e;
 sql_type_t for the typedef.
 What are BLOB and NULL?

> +struct sql_value_t
> +{
> +    int length;
> +    union
> +    {
> +        int i;
> +        double dbl;
> +        char* psz;
> +        void* ptr;
> +    } value;
> +};
 You could merge it with:
> +typedef struct sql_value_t sql_value_t;

> +    /** Create a statement object */
> +    sql_stmt_t* (*pf_prepare) ( sql_t* p_sql, const char* psz_fmt,
> +                                int i_length );
> +
> +    /** Bind parameters to a statement */
> +    int (*pf_bind) ( sql_t* p_sql, sql_stmt_t* p_stmt, sql_type_e type,
> +                             sql_value_t* p_value, int i_pos );
> +
> +    /** Run the prepared statement */
> +    int (*pf_run) ( sql_t* p_sql, sql_stmt_t* p_stmt );
> +
> +    /** Reset the prepared statement */
> +    int (*pf_reset) ( sql_t* p_sql, sql_stmt_t* p_stmt );
> +
> +    /** Destroy the statement object */
> +    int (*pf_finalize) ( sql_t* p_sql, sql_stmt_t* p_stmt );
> +
> +    /** Get the datatype for a specified column */
> +    int (*pf_gettype) ( sql_t* p_sql, sql_stmt_t* p_stmt, int i_col,
> +                        sql_type_e* type );
> +
> +    /** Get the data from a specified column */
> +    int (*pf_getcolumn) ( sql_t* p_sql, sql_stmt_t* p_stmt, int i_col,
> +                          sql_type_e type, sql_value_t *p_res );
I am not sure how to improve this, but it begins to make sql_t having an awful lot
of functions, and also to be hard to know which ones work together.
 Anyone has an idea?

> +struct sql_stmt_t
> +{
> +    sql_t* p_sql;
> +    sql_stmt_sys_t *p_sys;
> +};
 I don't think that explicitly declaring sql_stmt_t is needed.
It seems that is only needed by the plugin implementation.

> +/**
> + * @brief Prepare an sql statement
> + * @param p_sql The SQL object
> + * @param psz_fmt SQL query string
> + * @param i_length length of the string
> + * @return a sql_stmt_t pointer or NULL on failure
> + */
> +static inline sql_stmt_t* sql_Prepare( sql_t* p_sql, const char* psz_fmt,
> +        int i_length )
> +{
> +    return p_sql->pf_prepare( p_sql, psz_fmt, i_length );
> +}
 I don't understand what i_length is. If it is strlen(psz_fmt), then it is
useless, otherwise a more explicit documentation is needed.

> +/**
> + * @brief Bind arguments to a sql_stmt_t object
> + * @param p_sql The SQL object
> + * @param p_stmt Statement Object
> + * @param type Data type of the value
> + * @param p_value Value to be bound
> + * @param i_length For values which require a length
> + * @param i_pos Position at which the parameter should be bound
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +static inline int sql_BindGeneric( sql_t* p_sql, sql_stmt_t* p_stmt,
> +        sql_type_e type, sql_value_t* p_value, int i_pos )
 I think using int instead of sql_type_e is better for binary compatibility.
Is p_value modified by the function? (If not, a const is needed).

 I think more advanced wrapper would be helpful, they could completly
hide sql_value_t.

> +/**
> + * @brief Reset the SQL statement
> + * @param p_sql The SQL object
> + * @param p_stmt The sql statement object
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +static inline int sql_Reset( sql_t* p_sql, sql_stmt_t* p_stmt )
> +{
> +    return p_sql->pf_reset( p_sql, p_stmt );
> +}
 What exactly does this function? Is it the same than finalize+prepare?

> +/**
> + * @brief Get the datatype of the result of the column
> + * @param p_sql The SQL object
> + * @param p_stmt The sql statement object
> + * @param i_col The column
> + * @param type pointer to datatype of the given column
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +static inline int sql_GetColumnType( sql_t* p_sql, sql_stmt_t* p_stmt,
> +        int i_col, sql_type_e* type )
> +{
> +    return p_sql->pf_gettype( p_sql, p_stmt, i_col, type );
> +}
> +
> +/**
> + * @brief Get the column data
> + * @param p_sql The SQL object
> + * @param p_stmt The statement object
> + * @param i_col The column number
> + * @param type Datatype of result
> + * @param p_res The structure which contains the value of the result
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +static inline int sql_GetColumn( sql_t* p_sql, sql_stmt_t* p_stmt,
> +        int i_col, sql_type_e type, sql_value_t *p_res )
> +{
> +    return p_sql->pf_getcolumn( p_sql, p_stmt, i_col, type, p_res );
> +}
 Why is type needed? (Especially when sql_GetColumnType seems to be able
to retreive it).

> +#define sql_GetIntegerColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_INT, D )
> +#define sql_GetDoubleColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_DOUBLE, D )
> +#define sql_GetTextColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_TEXT, D )
> +#define sql_GetBlobColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_BLOB, D )
 I think more advanced wrapper would be helpful, they could completly hide sql_value_t.

-- 
fenrir



More information about the vlc-devel mailing list