Update mozStorageStatement.* to follow style guidelines

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted 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
Posted 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)
Posted 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]
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.
Attachment #374188 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth] → [needs new patch]
Posted patch v1.2Splinter Review
addresses review comment as well as fixed a few other spacing issues I noticed.
Attachment #374188 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/16e1a256b772
Status: ASSIGNED → RESOLVED
Closed: 10 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.