1557501 - Stop using [array] in nsIPrefBranch

Status: RESOLVED FIXED

(Core :: Preferences: Backend, task)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1557501. Stop using [array] in nsIPrefBranch. r=njn

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: probable comm-central fix: not tested, not compiled, caveat emptor

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76a2074020bd
Stop using [array] in nsIPrefBranch.  r=njn

Comment 5

[Jorg K (CEST = GMT+2)]

How to best replace the qsort here?
https://searchfox.org/comm-central/rev/a3437150e9d747de3eb28b4e45559ca32be7019c/mailnews/base/src/nsMsgTagService.cpp#352

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: c-c part

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

Comment 7

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: test.patch - with cloning stings in nsMsgTagService::GetAllTags()

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

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: c-c part

Thanks, Boris, that does the trick.

Comment 12

[Boris Zbarsky [: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.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: c-c part

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& ?

Comment 14

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 15

[Jorg K (CEST = GMT+2)]

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

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: c-c part

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

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/76a2074020bd

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/76a2074020bd

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ffde4bb2fe1b
C-C part: Adapt to array changes in nsIPrefBranch. r=jorgk,bz

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Jorg K (CEST = GMT+2)]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d00844c4eba4
C-C part: Fix typo in rev ffde4bb2fe1b. r=me