Closed Bug 489702 Opened 13 years ago Closed 13 years ago

Update mozStorageStatement.* to follow style guidelines


(Toolkit :: Storage, defect)

Not set





(Reporter: sdwilsh, Assigned: sdwilsh)




(1 file, 3 obsolete files)

Attached patch v1.0 (obsolete) — Splinter Review
This is the last file to be done - yay!
Attachment #374181 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
forgot to address an XXX comment to myself - new patch RSN!
Depends on: 489705
Attached patch v1.1 (obsolete) — Splinter Review
All better now!
Attachment #374181 - Attachment is obsolete: true
Attachment #374187 - Flags: review?(bugmail)
Attachment #374181 - Flags: review?(bugmail)
Attached patch v1.1 (obsolete) — Splinter Review
It's actually only better if I qrefresh first :/
Attachment #374187 - Attachment is obsolete: true
Attachment #374188 - Flags: review?(bugmail)
Attachment #374187 - Flags: review?(bugmail)
Comment on attachment 374188 [details] [diff] [review]

>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)
>+  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)
>+  // 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)
>+  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);

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.
Attachment #374188 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth] → [needs new patch]
Attached patch v1.2Splinter Review
addresses review comment as well as fixed a few other spacing issues I noticed.
Attachment #374188 - Attachment is obsolete: true
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs new patch]
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.