Closed Bug 1557501 Opened 1 year ago Closed 1 year ago

Stop using [array] in nsIPrefBranch

Categories

(Core :: Preferences: Backend, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

No description provided.
Flags: needinfo?(jorgk)

Thanks, I'll take it from here. Looks like the JS changes are limited and the compiler will tell me if there's something wrong/missing in the C++ code.

Flags: needinfo?(jorgk)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76a2074020bd
Stop using [array] in nsIPrefBranch.  r=njn
Attached patch c-c part (obsolete) — Splinter Review

This compiles, should be OK, but not sure about the qsort. Maybe Nicholas is still around.

Attachment #9070387 - Attachment is obsolete: true
Attachment #9070505 - Flags: feedback?(n.nethercote)
Attachment #9070505 - Flags: feedback?(bzbarsky)

We have two "tag tests", comm/mailnews/base/test/unit/test_nsIMsgTagService.js, comm/mailnews/base/test/unit/test_searchTag.js. I'll run those when the compile is done.

EDIT: They crash :-(

In absence of a better solution for qsort, let's copy the strings.

Comment on attachment 9070505 [details] [diff] [review]
c-c part

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

::: mailnews/base/src/nsMsgTagService.cpp
@@ +349,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    // sort them by key for ease of processing
> +  qsort(&prefList, prefList.Length(), sizeof(nsCString), CompareMsgTagKeys);

That looks wrong, there is no .get() to get to the pointer array, and `qsort(&prefList[0], prefList.Length(), sizeof(nsCString *), CompareMsgTagKeys);` doesn't work either. I wonder whether it's possible.
Attachment #9070505 - Flags: feedback?(n.nethercote)

How to best replace the qsort here?

Sort() on the nsTArray. Looks like CompareMsgTagKeys is a bog-standard lexicographic comparator, so you don't even need a custom comparator object, since strings define == and < operators. Just do:

   prefList.Sort();

and you're done.

Attachment #9070505 - Flags: feedback?(bzbarsky) → feedback-
Attached patch c-c part (obsolete) — Splinter Review

Thanks, Boris, that does the trick.

Attachment #9070505 - Attachment is obsolete: true
Attachment #9070508 - Attachment is obsolete: true
Attachment #9070595 - Flags: feedback?(bzbarsky)
Comment on attachment 9070595 [details] [diff] [review]
c-c part

>+++ b/mailnews/base/src/nsMsgTagService.cpp
>@@ -339,67 +333,63 @@ NS_IMETHODIMP nsMsgTagService::GetAllTag
>-  for (uint32_t i = prefCount; i--;) {
>+  for (auto &pref : prefList) {

The old code traversed the list back to front.  The new code does front-to-back.  No idea whether this matters, but you can throw in a Reversed() to keep the old behavior.

>-    char *info = strrchr(prefList[i], '.');
>+    const char *info = strrchr(pref.get(), '.');
>     if (info) {
>-      nsAutoCString key(Substring(prefList[i], info));
>+      nsAutoCString key(Substring(pref.get(), info));

This is fine as far as it goes, but you could do the same with less copying:

  int32_t dotLoc = pref.RFindChar('.');
  if (dotLoc != kNotFound) {
    auto& key = Substring(pref, dotLoc)

Either way, though.
Attachment #9070595 - Flags: feedback?(bzbarsky) → feedback+
Attached patch c-c part (obsolete) — Splinter Review

Updated with latest comments, thanks, Boris. Less copying? Substring will copy, there's no way around it. And the search is the same, Rfind and strrchr() do the same thing in principal, no?

Any objection to

-    char *info = strrchr(prefList[i], '.');
-    if (info) {
-      nsAutoCString key(Substring(prefList[i], info));
+    int32_t dotLoc = pref.RFindChar('.');
+    if (dotLoc != kNotFound) {
+      nsAutoCString key(Substring(pref, dotLoc));

instead of using the auto& ?

Attachment #9070595 - Attachment is obsolete: true
Attachment #9070611 - Flags: review+

Hmm, something wrong here now, tests fail (also with auto& as suggested). What's the difference between Substring with a number and a char* argument? Strings in Mozilla, it will take me another 10 years to learn them :-(

Oh, apparently Substring doesn't copy, so the auto& is more efficient.

Attached patch c-c partSplinter Review

OK, the Substring() was missing a 0. Now test pass.

Attachment #9070611 - Attachment is obsolete: true
Attachment #9070619 - Flags: review+
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ffde4bb2fe1b
C-C part: Adapt to array changes in nsIPrefBranch. r=jorgk,bz

Oh, right, Substring(ptr, ptr) returns that range... So you really wanted StringHead(pref, dotLoc).

Sigh, Mozilla strings, I hope we can survive with Substring(pref, 0, dotLoc). Looks like Substring from 0 is used more frequently, maybe StringHead is newer.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d00844c4eba4
C-C part: Fix typo in rev ffde4bb2fe1b. r=me
Regressions: 1559407
You need to log in before you can comment on or make changes to this bug.