Closed Bug 777770 Opened 12 years ago Closed 9 years ago

get rid of nsVoidArray from /mailnews

Categories

(MailNews Core :: Backend, defect)

defect
Not set
blocker

Tracking

(thunderbird39 unaffected, thunderbird40 fixed, thunderbird41 fixed)

RESOLVED FIXED
Thunderbird 41.0
Tracking Status
thunderbird39 --- unaffected
thunderbird40 --- fixed
thunderbird41 --- fixed

People

(Reporter: sgautherie, Assigned: ssitter)

References

(Blocks 2 open bugs, )

Details

(Keywords: dogfood)

Attachments

(3 files, 2 obsolete files)

"Found 122 matching lines in 39 files"
Can you explain why it should be removed and what it needs to be changed to?
See bug 485693 (and bug 474369)...
Blocks: 819090
Severity: normal → blocker
"Found 119 matching lines in 34 files"
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).
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.
Compiled ;-)
Keywords: dogfood
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
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.
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch wip still (obsolete) — Splinter Review
Attachment #8604506 - Attachment is obsolete: true
Attachment #8604506 - Flags: feedback?(rkent)
Attachment #8604581 - Flags: feedback?(rkent)
Attached file errlog_8604581.txt
Error log for attachment 8604581 [details] [diff] [review] on SM-Trunk Linux x86_64.
This patch compiles on my Windows system, maybe you can check it on your systems.
Stefan: I'll check your recent patch.
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?
With attachment 8604875 [details] [diff] [review] building SM-Trunk Linux x86_64 succeeds. I am now using this SM.
Attachment #8604581 - Attachment is obsolete: true
Attachment #8604581 - Flags: feedback?(rkent)
Assignee: ewong → ssitter
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 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+
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 on attachment 8604875 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 1

[Approval Request Comment]

Build busted.
Attachment #8604875 - Flags: approval-comm-aurora?
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.
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.
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)
Attachment #8604875 - Attachment description: WIP → Replace nsVoidArray with nsTArray, part 1
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 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+
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Comment on attachment 8605427 [details] [diff] [review]
Replace nsVoidArray with nsTArray, part 2

[Triage Comment]
Attachment #8605427 - Flags: approval-comm-aurora+
(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.
Blocks: 1164839
You need to log in before you can comment on or make changes to this bug.