Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(3 attachments)

Because iterators are so much nicer than enumerate functions.

There are three occurrences of EnumerateRead() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::UserData() rather than nsBaseHashtable::Iterator::Data(). (The latter should be used when replacing nsBaseHashtable::Enumerate()).
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8679783 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Review of attachment 8679783 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/mozStorageBindingParams.cpp
@@ +197,5 @@
>    // hashtable.
>    if (!mNamedParameters.Count())
>      return BindingParams::bind(aStatement);
>  
> +  nsCOMPtr<mozIStorageError> err = nullptr;

afaik, there's no need to assign nullptr to nsCOMPtr, it should already be initialized to that, unless it changed in the last months

@@ +226,5 @@
> +      // We had an error while trying to bind.  Now we need to create an error
> +      // object with the right message.  Note that we special case
> +      // SQLITE_MISMATCH, but otherwise get the message from SQLite.
> +      const char *msg = "Could not covert nsIVariant to SQLite type.";
> +      if (rc != SQLITE_MISMATCH)

nit: while here, please brace this if...

@@ +229,5 @@
> +      const char *msg = "Could not covert nsIVariant to SQLite type.";
> +      if (rc != SQLITE_MISMATCH)
> +        msg = ::sqlite3_errmsg(::sqlite3_db_handle(aStatement));
> +
> +      err = new Error(rc, msg);

...and then you can remove the newline before this.
Attachment #8679783 - Flags: review?(mak77) → review+
Attachment #8679784 - Flags: review?(mak77) → review+
Comment on attachment 8679785 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators

Review of attachment 8679785 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/mozStorageConnection.cpp
@@ +1317,5 @@
> +  for (auto iter = mFunctions.Iter(); !iter.Done(); iter.Next()) {
> +    const nsACString &key = iter.Key();
> +    Connection::FunctionInfo data = iter.UserData();
> +
> +    NS_PRECONDITION(data.type == Connection::FunctionInfo::SIMPLE ||

please change this to a MOZ_ASSERT, it would be a coding mistake.

@@ +1324,5 @@
> +
> +    if (data.type == Connection::FunctionInfo::SIMPLE) {
> +      mozIStorageFunction *function =
> +        static_cast<mozIStorageFunction *>(data.function.get());
> +      (void)aClone->CreateFunction(key, data.numArgs, function);

while here, this should probably warn and not just swallow any error... could define a DebugOnly<rv> and warn something like "Failed to copy function '$key' to cloned connection".

@@ +1328,5 @@
> +      (void)aClone->CreateFunction(key, data.numArgs, function);
> +    } else {
> +      mozIStorageAggregateFunction *function =
> +        static_cast<mozIStorageAggregateFunction *>(data.function.get());
> +      (void)aClone->CreateAggregateFunction(key, data.numArgs, function);

ditto
Attachment #8679785 - Flags: review?(mak77) → review+
Thanks for this cleanup!
https://hg.mozilla.org/integration/mozilla-inbound/rev/582ecb12478ae567b439bb5aa01a1ac3b9551e61
Bug 1186791 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15aad0a5b3cb1695bacaf3e4ecb40339cb7a2a1c
Bug 1186791 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/234e629e87cc1761289130f85039bba64a6ea07e
Bug 1186791 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in storage/ with iterators. r=mak.
https://hg.mozilla.org/mozilla-central/rev/582ecb12478a
https://hg.mozilla.org/mozilla-central/rev/15aad0a5b3cb
https://hg.mozilla.org/mozilla-central/rev/234e629e87cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.