Hi,<br><br><div class="gmail_quote">On Tue, Oct 20, 2009 at 10:48 PM, Laurent Aimar <span dir="ltr"><<a href="mailto:fenrir@via.ecp.fr">fenrir@via.ecp.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi,<br>
<br>
> +#define VLC_SQL_ROW 1<br>
> +#define VLC_SQL_DONE 2<br>
What are they used for? A doxy comment would be usefull.<br>
<br></blockquote><div>They're used as return values for sql_Run(). Documented.<br><br><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">> What are they used for? A doxy comment would be usefull.<br>
Also: enums. <br></blockquote>
Really necessary?<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> +typedef enum {<br>
> + SQL_INT,<br>
> + SQL_DOUBLE,<br>
> + SQL_TEXT,<br>
> + SQL_BLOB,<br>
> + SQL_NULL<br>
> +} sql_type_e;<br>
sql_type_t for the typedef.<br>
What are BLOB and NULL?<br>
<br></blockquote><div>BLOBs are binary objects. NULLs are treated specially in databases, as 0 is not NULL in a database<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> +struct sql_value_t<br>
> +{<br>
> + int length;<br>
> + union<br>
> + {<br>
> + int i;<br>
> + double dbl;<br>
> + char* psz;<br>
> + void* ptr;<br>
> + } value;<br>
> +};<br>
You could merge it with:<br>
> +typedef struct sql_value_t sql_value_t;<br>
<br></blockquote><div>I didn't because the other typedefs arent.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
> +struct sql_stmt_t<br>
> +{<br>
> + sql_t* p_sql;<br>
> + sql_stmt_sys_t *p_sys;<br>
> +};<br>
I don't think that explicitly declaring sql_stmt_t is needed.<br>
It seems that is only needed by the plugin implementation.<br></blockquote><div>Needed only by the plugin implementation meaning? It's required by users of<br>the sql module. They hold references to an sql_stmt_t object and the internals<br>
vary with the different modules. I think I'm missing something?<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
</blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> +/**<br>
> + * @brief Prepare an sql statement<br>
> + * @param p_sql The SQL object<br>
> + * @param psz_fmt SQL query string<br>
> + * @param i_length length of the string<br>
> + * @return a sql_stmt_t pointer or NULL on failure<br>
> + */<br>
> +static inline sql_stmt_t* sql_Prepare( sql_t* p_sql, const char* psz_fmt,<br>
> + int i_length )<br>
> +{<br>
> + return p_sql->pf_prepare( p_sql, psz_fmt, i_length );<br>
> +}<br>
I don't understand what i_length is. If it is strlen(psz_fmt), then it is<br>
useless, otherwise a more explicit documentation is needed.<br></blockquote><div><br>I've documented it. jpd and Paul Corke are right. It is perfectly ok to put 0s in a database because they are treated differently from NULL. Using a -ve length will select the psz_fmt upto the null terminator though.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
> +/**<br>
> + * @brief Bind arguments to a sql_stmt_t object<br>
> + * @param p_sql The SQL object<br>
> + * @param p_stmt Statement Object<br>
> + * @param type Data type of the value<br>
> + * @param p_value Value to be bound<br>
> + * @param i_length For values which require a length<br>
> + * @param i_pos Position at which the parameter should be bound<br>
> + * @return VLC_SUCCESS or VLC_EGENERIC<br>
> + */<br>
> +static inline int sql_BindGeneric( sql_t* p_sql, sql_stmt_t* p_stmt,<br>
> + sql_type_e type, sql_value_t* p_value, int i_pos )<br>
I think using int instead of sql_type_e is better for binary compatibility.<br></blockquote><div>Type safety?<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Is p_value modified by the function? (Ifnot, a const is needed).<br></blockquote><div>No. Added.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
I think more advanced wrapper would be helpful, they could completly<br>
hide sql_value_t.<br>
<br>
> +/**<br>
> + * @brief Reset the SQL statement<br>
> + * @param p_sql The SQL object<br>
> + * @param p_stmt The sql statement object<br>
> + * @return VLC_SUCCESS or VLC_EGENERIC<br>
> + */<br>
> +static inline int sql_Reset( sql_t* p_sql, sql_stmt_t* p_stmt )<br>
> +{<br>
> + return p_sql->pf_reset( p_sql, p_stmt );<br>
> +}<br>
What exactly does this function? Is it the same than finalize+prepare?<br></blockquote><div>No. It unbinds the values that were "bound" to the statement. Documented. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
</blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> +/**<br>
> + * @brief Get the datatype of the result of the column<br>
> + * @param p_sql The SQL object<br>
> + * @param p_stmt The sql statement object<br>
> + * @param i_col The column<br>
> + * @param type pointer to datatype of the given column<br>
> + * @return VLC_SUCCESS or VLC_EGENERIC<br>
> + */<br>
> +static inline int sql_GetColumnType( sql_t* p_sql, sql_stmt_t* p_stmt,<br>
> + int i_col, sql_type_e* type )<br>
> +{<br>
> + return p_sql->pf_gettype( p_sql, p_stmt, i_col, type );<br>
> +}<br>
> +<br>
> +/**<br>
> + * @brief Get the column data<br>
> + * @param p_sql The SQL object<br>
> + * @param p_stmt The statement object<br>
> + * @param i_col The column number<br>
> + * @param type Datatype of result<br>
> + * @param p_res The structure which contains the value of the result<br>
> + * @return VLC_SUCCESS or VLC_EGENERIC<br>
> + */<br>
> +static inline int sql_GetColumn( sql_t* p_sql, sql_stmt_t* p_stmt,<br>
> + int i_col, sql_type_e type, sql_value_t *p_res )<br>
> +{<br>
> + return p_sql->pf_getcolumn( p_sql, p_stmt, i_col, type, p_res );<br>
> +}<br>
Why is type needed? (Especially when sql_GetColumnType seems to be able<br>
to retreive it).<br></blockquote><div>Because databases might do type casting. If there is an INT at i_col = 1, but you ask for a string, it will perform an itoa() and return the value.<br>Atleast, that is how the sqlite interface is implemented.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
> +#define sql_GetIntegerColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_INT, D )<br>
> +#define sql_GetDoubleColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_DOUBLE, D )<br>
> +#define sql_GetTextColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_TEXT, D )<br>
> +#define sql_GetBlobColumn( A, B, C, D ) sql_GetColumn( A, B, C, SQL_BLOB, D )<br>
I think more advanced wrapper would be helpful, they could completly hide sql_value_t.<br></blockquote><div> </div><div>I'm not sure why one would want to hide the type of the value, because that was one of the reasons the prepared statement interface is being included. So that there may be stronger type checking.<br>
<br></div></div><br><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote">
> I am not sure how to improve this, but it begins to make sql_t having<br>
> an awful lot of functions, and also to be hard to know which ones work<br>
> together. Anyone has an idea?<br><br>Prepared statements take a few extra functions, but perhaps one can<br>
merge functionality with the non-prepared versions. Though if that<br>
causes lots of extra complexity, then perhaps not.<br clear="all"></blockquote><div>By non-prepared versions I assume you mean the Query(sqlquery,...) interface. <br></div>
The non-prepared versions utilise the prepared statement interface internally. The point of having the prepared statement interface was to have control over values being bound. Trying to do this with the the non-prepared versions doesn't make sense, because it brings back the sql injection problem.<br>
I'm not sure how you can combine "binding" with sqlquery. Functionality very close to this already exists in the sql_Printf() interface.<br><br>-- <br>Regards,<br>Srikanth Raju<br>