489702 - Update mozStorageStatement.* to follow style guidelines

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This is the last file to be done - yay!

Comment 1

[Shawn Wilsher :sdwilsh]

forgot to address an XXX comment to myself - new patch RSN!

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

All better now!

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

It's actually only better if I qrefresh first :/

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 374188 [details] [diff] [review]
v1.1

>diff --git a/storage/src/mozStorageStatement.cpp b/storage/src/mozStorageStatement.cpp
>--- a/storage/src/mozStorageStatement.cpp
>+++ b/storage/src/mozStorageStatement.cpp
>+Statement::initialize(Connection *aDBConnection,
>+                      const nsACString &aSQLStatement)
...
>+  for (PRUint32 i = 0; i < mResultColumnCount; i++) {
>+      const char *name = sqlite3_column_name(mDBStatement, i);
                           ^
:: omitted


>+      mColumnNames.AppendElement(nsDependentCString(name));

Should this technically be void-cast?  The call returns something (a pointer to the added element), but the call can't fail and so is not a result indicator.


>+Statement::Clone(mozIStorageStatement **_statement)
> {
...
>-    NS_ADDREF(*_retval = mss);
>+  statement.forget(_statement);
>+  NS_ADDREF(*_statement = statement);

I don't think you want that NS_ADDREF anymore.


It's interesting that the previous code used NS_ENSURE_ARG_POINTER on out-parameters but only in a few places.  Weird.


>+Statement::BindUTF8StringParameter(PRUint32 aParamIndex,
>+                                   const nsACString &aValue)
>+{
>+  if (!mDBStatement)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  int srv = ::sqlite3_bind_text(mDBStatement, aParamIndex + 1,
>+                                nsPromiseFlatCString(aValue).get(),
                                  ^^
You've been losing the 'ns' on these in general, might as well here too.


>+Statement::BindStringParameter(PRUint32 aParamIndex,
>+                               const nsAString &aValue)
>+{
...
>+  int srv = ::sqlite3_bind_text16(mDBStatement, aParamIndex + 1,
>+                                  nsPromiseFlatString(aValue).get(),

and here


>+Statement::GetUTF8String(PRUint32 aIndex,
>+                         nsACString &_value)
>+{
>+  if (!mDBStatement)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  // Get type of Index will check aIndex for us, so we don't have to.
>+  PRInt32 type;
>+  (void)GetTypeOfIndex(aIndex, &type);

I do not the loss of the NS_ENSURE_SUCCESS here without gaining an
ENSURE_INDEX_VALUE.  Illegal index => type has random value => wonky-town.
The mExecuting check also gets lost.  It makes a liar out of the comment too.

>+Statement::GetString(PRUint32 aIndex,
>+                     nsAString &_value)

same deal

>+Statement::GetBlob(PRUint32 aIndex,
>+                   PRUint32 *_size,
>+                   PRUint8 **_blob)
>+{
>+  if (!mDBStatement)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  ENSURE_INDEX_VALUE(aIndex, mResultColumnCount);
>+
>+  if (!mExecuting)
>+     return NS_ERROR_UNEXPECTED;
>+
>+  int size = ::sqlite3_column_bytes(mDBStatement, aIndex);
>+  void *blob = nsMemory::Clone(::sqlite3_column_blob(mDBStatement, aIndex),
>+                               size);
>+  NS_ENSURE_TRUE(blob, NS_ERROR_OUT_OF_MEMORY);

I'm not sure this is reliable.  You removed the blobsize == 0 special case path,
but the revised code only fails to explode if nsMemory::Clone does not return
NULL but instead a sentinel value or actual zero-length allocation.
nsMemory::Clone calls NS_Alloc which seems to call PR_Malloc which defines its
malloc semantics as equivalent to libc.  The malloc docs on my system says:
"If size is 0, then malloc() returns either NULL, or a unique pointer value
that can later be successfully passed to free()."

It looks like jemalloc as built for mozilla probably bumps the size up to 1
which means a non-null pointer, which means workingness under jemalloc.

I am assuming XPConnect handles things where you give it a real pointer it
should free and a size of 0.

Please either add a comment or put things back the way they were.


>+Statement::GetIsNull(PRUint32 aIndex,
>+                     PRBool *_isNull)
...
>+    // Get type of Index will check aIndex for us, so we don't have to.
>+    PRInt32 type;
>+    (void)GetTypeOfIndex(aIndex, &type);

Same deal as Get*String.


r+ with concerns addressed.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

addresses review comment as well as fixed a few other spacing issues I noticed.

Comment 6

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/16e1a256b772