get rid of nsVoidArray from /mailnews

RESOLVED FIXED in Thunderbird 41.0

Status

MailNews Core
Backend
--
blocker
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: sgautherie, Assigned: ssitter)

Tracking

(Blocks: 2 bugs, {dogfood})

Trunk
Thunderbird 41.0
dogfood
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird39 unaffected, thunderbird40 fixed, thunderbird41 fixed)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
"Found 122 matching lines in 39 files"

Comment 1

6 years ago
Can you explain why it should be removed and what it needs to be changed to?
(Reporter)

Comment 2

6 years ago
See bug 485693 (and bug 474369)...

Updated

3 years ago
Blocks: 819090

Updated

3 years ago
Severity: normal → blocker

Comment 3

3 years ago
"Found 119 matching lines in 34 files"
Comment hidden (obsolete)
Comment hidden (obsolete)
Is there a plan to fix this?
Does someone know how to revert back to a working rev, on #maildev they said:
cd mozilla && hg up <rev> (I don't know what rev works though).

Comment 7

3 years ago
If you go to treeherder https://treeherder.mozilla.org/#/jobs?repo=comm-central and click n the green B from a recently completed job, you can see which mozilla rev was used successfully there. I see 617dbce26726
Thanks, hg up 617dbce26726 brings the missing nsVoidArray.h back, let's see whether the rest compiles.

Updated

3 years ago
Keywords: dogfood

Comment 10

3 years ago
I was hoping one of you would be motivated to fix this. Anyone working on it, or willing to work on it?
(In reply to Kent James (:rkent) from comment #10)
> I was hoping one of you would be motivated to fix this. Anyone working on
> it, or willing to work on it?

Working on it.
Assignee: nobody → ewong

Updated

3 years ago
Duplicate of this bug: 1163323
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Actually, it was removed in bug 819090.

How is the progress? We are really hampered at the moment.
(In reply to Jorg K from comment #47)
> Actually, it was removed in bug 819090.
> 
> How is the progress? We are really hampered at the moment.

I'm sorry for the slow going.

Currently working on it.
Created attachment 8604506 [details] [diff] [review]
wip

Current WIP.

It doesn't build as I'm getting the following:

1:21.14 Warning: C4180 in e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h: qualifier applied to function type has no meaning; ignored
 1:21.14 e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h(1647) : warning C4180: qualifier applied to function type has no meaning; ignored
 1:21.14         e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h(1660) : see reference to function template instantiation 'int nsTArray_Impl<E,nsTArrayInfallibleAllocator>::Compare<Comparator>(const void *,const void *,void *)' being compiled
 1:21.14         with
 1:21.14         [
 1:21.14             E=void *
 1:21.14 ,            Comparator=int (const void *,const void *,void *)
 1:21.14         ]
 1:21.14         e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h(1660) : see reference to function template instantiation 'int nsTArray_Impl<E,nsTArrayInfallibleAllocator>::Compare<Comparator>(const void *,const void *,void *)' being compiled
 1:21.14         with
 1:21.14         [
 1:21.14             E=void *
 1:21.14 ,            Comparator=int (const void *,const void *,void *)
 1:21.14         ]
 1:21.14         e:/mozstuff/mozprgs/trunk/mailnews/addrbook/src/nsAbView.cpp(751) : see reference to function template instantiation 'void nsTArray_Impl<E,nsTArrayInfallibleAllocator>::Sort<int(const void *,const void *,void *)>(Comparator (__cdecl &))' being compiled
 1:21.14         with
 1:21.14         [
 1:21.14             E=void *
 1:21.14 ,            Comparator=int (const void *,const void *,void *)
 1:21.14         ]
 1:21.14         e:/mozstuff/mozprgs/trunk/mailnews/addrbook/src/nsAbView.cpp(751) : see reference to function template instantiation 'void nsTArray_Impl<E,nsTArrayInfallibleAllocator>::Sort<int(const void *,const void *,void *)>(Comparator (__cdecl &))' being compiled
 1:21.14         with
 1:21.14         [
 1:21.14             E=void *
 1:21.14 ,            Comparator=int (const void *,const void *,void *)
 1:21.14         ]
 1:21.14 e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h(1650) : error C2227: left of '->LessThan' must point to class/struct/union/generic type
 1:21.14         type is 'int (__cdecl *)(const void *,const void *,void *)'
 1:21.14 e:\mozstuff\mozprgs\trunk\objdir\dist\include\nsTArray.h(1650) : error C2227: left of '->Equals' must point to class/struct/union/generic type
 1:21.14         type is 'int (__cdecl *)(const void *,const void *,void *)'
 1:21.14 
 1:21.15 In the directory  /e/mozstuff/mozprgs/trunk/objdir/mailnews/addrbook/src
 1:21.15 The following command failed to execute properly:
 1:21.15 e:/mozstuff/mozprgs/trunk/objdir/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FonsAbView.obj -c -I../../../dist/stl_wrappers -DMOZ_LDAP_XPCOM -DNOMINMAX -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -Ie:/mozstuff/mozprgs/trunk/mailnews/addrbook/src -I. -I../../../dist/include -Ie:/mozstuff/mozprgs/trunk/objdir/dist/include/nspr -Ie:/mozstuff/mozprgs/trunk/objdir/dist/include/nss -MD -FI ../../../dist/include/mozilla-config.h -DMOZILLA_CLIENT -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -Fdgenerated.pdb e:/mozstuff/mozprgs/trunk/mailnews/addrbook/src/nsAbView.cpp
 1:21.16 e:/mozstuff/mozprgs/trunk/mozilla/config/rules.mk:932: recipe for target 'nsAbView.obj' failed
 1:21.16 mozmake.EXE[4]: *** [nsAbView.obj] Error 1
 1:21.16 e:/mozstuff/mozprgs/trunk/mozilla/config/recurse.mk:70: recipe for target 'mailnews/addrbook/src/target' failed
 1:21.16 mozmake.EXE[3]: *** [mailnews/addrbook/src/target] Error 2
 1:21.16 mozmake.EXE[3]: *** Waiting for unfinished jobs....
Attachment #8604506 - Flags: feedback?(rkent)
Comment on attachment 8604506 [details] [diff] [review]
wip

Actually, I have managed to get past that minor bump with the help from
Neil over irc.  will update the patch when I can, but feedback is certainly
appreciated.
Created attachment 8604581 [details] [diff] [review]
wip still
Attachment #8604506 - Attachment is obsolete: true
Attachment #8604506 - Flags: feedback?(rkent)
Attachment #8604581 - Flags: feedback?(rkent)

Comment 52

3 years ago
Created attachment 8604632 [details]
errlog_8604581.txt

Error log for attachment 8604581 [details] [diff] [review] on SM-Trunk Linux x86_64.
(Assignee)

Comment 53

3 years ago
Created attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

This patch compiles on my Windows system, maybe you can check it on your systems.

Comment 54

3 years ago
Stefan: I'll check your recent patch.

Comment 55

3 years ago
Comment on attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

Yes compiles for me, pushed to try https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=656db41521fb

Stefan, if you are happy with your patch can you please request review, or do any needed updates?

Comment 56

3 years ago
With attachment 8604875 [details] [diff] [review] building SM-Trunk Linux x86_64 succeeds. I am now using this SM.

Updated

3 years ago
Attachment #8604581 - Attachment is obsolete: true
Attachment #8604581 - Flags: feedback?(rkent)

Updated

3 years ago
Assignee: ewong → ssitter
(Assignee)

Comment 57

3 years ago
Comment on attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

In my opinion the patch is not complete because it currently introduces several new warnings like signed/unsigned mismatch or because there are many remains of now unnecessary casts to and from void*.

But maybe it is complete enough to fix the build breakage therefore review would be appreciated. Some changes like the new CardComperator I gratefully took over from Edmunds patch and other changes like in the idl I just did by comparing with existing code. It compiles but I don't know if everything works as before :)
Attachment #8604875 - Flags: review?(rkent)

Comment 58

3 years ago
Comment on attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

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

Overall looks good. I think that we need to get this landed as it is blocking critical stuff. Could you file a followup bug to do any remaining cleanup?

r=me with few nits fixed.

::: mailnews/addrbook/src/nsAbView.cpp
@@ +683,5 @@
> +  private:
> +    SortClosure *m_closure;
> +
> +  public:
> +    void setclosure(SortClosure *closure) { m_closure = closure; };

Please call this SetClosure

@@ +684,5 @@
> +    SortClosure *m_closure;
> +
> +  public:
> +    void setclosure(SortClosure *closure) { m_closure = closure; };
> +    SortClosure * retclosure() { return m_closure; };

You never call retclosure so just remove this.

::: mailnews/base/src/nsMsgDBView.h
@@ +35,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsIWeakReferenceUtils.h"
>  #define MESSENGER_STRING_URL       "chrome://messenger/locale/messenger.properties"
>  
> +template <class> class nsTArray;

You don't need this, please delete.

::: mailnews/imap/src/nsIMAPNamespace.cpp
@@ +113,5 @@
>  
>  int nsIMAPNamespaceList::GetNumberOfNamespaces(EIMAPNamespaceType type)
>  {
>  	int nodeIndex = 0, count = 0;
> +	for (nodeIndex=m_NamespaceList.Length()-1; nodeIndex >= 0; nodeIndex--)

Nit: space around "=" and "-1"

@@ +133,5 @@
>  	if (!ns->GetIsNamespaceFromPrefs())
>  	{
>  		int nodeIndex;
>          // iterate backwards because we delete elements
> +		for (nodeIndex=m_NamespaceList.Length()-1; nodeIndex >= 0; nodeIndex--)

Nit: space around "=" and "-1" (and more below)

::: mailnews/import/oexpress/nsOEScanBoxes.cpp
@@ +476,2 @@
>    {
>      pEntry = (MailboxEntry *) m_pendingChildArray.ElementAt(i);

You don't need the (MailboxEntry *)

@@ +579,5 @@
>      }
>    }
>  
> +  if (m_entryArray.Length() > 0) {
> +    pEntry = (MailboxEntry *)m_entryArray.ElementAt(m_entryArray.Length() - 1);

You don't need the (MailboxEntry *)

@@ +644,5 @@
>      }
>    }
>  
> +  if (m_entryArray.Length() > 0) {
> +    pEntry = (MailboxEntry *)m_entryArray.ElementAt(m_entryArray.Length() - 1);

You don't need the (MailboxEntry *)

@@ +660,1 @@
>          pBox = (MailboxEntry *) m_entryArray.ElementAt(0);

You don't need the (MailboxEntry *)

@@ +708,1 @@
>          pBox = (MailboxEntry *) m_entryArray.ElementAt(0);

You don't need the (MailboxEntry *)

@@ +767,2 @@
>    for (int32_t i = 0; i < max; i++) {
>      MailboxEntry *pEntry = (MailboxEntry *) m_entryArray.ElementAt(i);

You don't need the (MailboxEntry *)
Attachment #8604875 - Flags: review?(rkent) → review+

Comment 59

3 years ago
I finished the nits, and pushed:

https://hg.mozilla.org/comm-central/rev/d294256abf7b

Sorry to rush this, but the closed tree is causing major disruption.

Comment 60

3 years ago
Comment on attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

[Approval Request Comment]

Build busted.
Attachment #8604875 - Flags: approval-comm-aurora?

Updated

3 years ago
status-thunderbird39: --- → unaffected
status-thunderbird40: --- → affected
status-thunderbird41: --- → fixed
(Assignee)

Comment 61

3 years ago
Bah, I just finished creating a patch that contains the changes as requested by you + the outstanding fixes mentioned by me but now my patch contains only merge errors.

Comment 62

3 years ago
Sorry about that. I'm on a fairly tight schedule trying to get the (already late) Tb 38 finished, and I really need to get some pushes done.
(Assignee)

Comment 63

3 years ago
Created attachment 8605427 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 2

If I got it right this should be the difference to the original patch to be applied on top of it.
Attachment #8605427 - Flags: review?(rkent)
(Assignee)

Updated

3 years ago
Attachment #8604875 - Attachment description: WIP → Replace nsVoidArray with nsTArray, part 1
(Assignee)

Comment 64

3 years ago
Or to be more precise attachment #8605427 [details] [diff] [review] should be applied on top of the modified version of attachment #8604875 [details] [diff] [review] that was pushed to https://hg.mozilla.org/comm-central/rev/d294256abf7b

Comment 65

3 years ago
Comment on attachment 8605427 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 2

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

LGTM
Attachment #8605427 - Flags: review?(rkent) → review+

Updated

3 years ago
Keywords: checkin-needed

Comment 67

3 years ago
For future reference, please update the uuid in all interfaces you touch:

remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIPop3Protocol in changeset 3eb84dd9febc
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIPop3Protocol
Keywords: checkin-needed

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0

Comment 68

3 years ago
Comment on attachment 8605427 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 2

[Triage Comment]
Attachment #8605427 - Flags: approval-comm-aurora+

Updated

3 years ago
status-thunderbird40: affected → fixed
(In reply to Stefan Sitter from comment #57)
> Comment on attachment 8604875 [details] [diff] [review]
> Replace nsVoidArray with nsTArray, part 1
> 
> In my opinion the patch is not complete because it currently introduces
> several new warnings like signed/unsigned mismatch or because there are many
> remains of now unnecessary casts to and from void*.
> 
> But maybe it is complete enough to fix the build breakage therefore review
> would be appreciated. Some changes like the new CardComperator I gratefully
> took over from Edmunds patch and other changes like in the idl I just did by
> comparing with existing code. It compiles but I don't know if everything
> works as before :)

CardComparator was Neil's idea.
(Reporter)

Updated

3 years ago
Blocks: 1164839
You need to log in before you can comment on or make changes to this bug.