1317040 - Port Bug 1315812 | Mark nsISupportsArray, nsICollection, nsIEnumerator as deprecated| to Mailnews: error C2220: warning treated as error - warning C4996: 'nsICollection': was declared deprecated

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(58): error C2220: warning treated as error - no 'object' file generated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(58): warning C4996: 'nsICollection': was declared deprecated

...

c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mozilla/config/rules.mk:951: recipe for target 'ImportOutFile.obj' failed
mozmake.exe[5]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/objdir-tb/mailnews/import/src'
c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mozilla/config/recurse.mk:71: recipe for target 'mailnews/import/src/target' failed
mozmake.exe[5]: *** [ImportOutFile.obj] Error 2
mozmake.exe[4]: *** [mailnews/import/src/target] Error 2
mozmake.exe[4]: *** Waiting for unfinished jobs....

Comment 1

[:aceman]

I've seen they mark this deprecated. I didn't expect it to break the build. OK, so we must fix it quickly.

Anyway, in bug 1315812 that did this, there is sample code for disabling the warning in specific files.

But we get more hosed if the same effect happens for nsISupportArray (which was marked deprecated too). That one is sprinkled all over the code due to filters using it.

Comment 2

[Jorg K (CEST = GMT+2)]

Anything that will keep us going with get an r+ from me especially that the other platforms are building and not stopping on the warning. The current situation makes no sense.

I thought nsISupportArray was mostly gone?

Comment 3

[:aceman]

nsISupportsArray is mostly gone (but not totally, even m-c had to disable the warnings for some files), but there is a one big offender remaining, that is nsIMsgFilter::searchTerms . Due to that there are about hundred places that still reference nsISupportsArray. There is bug 857230 for it. It is a large patch/project but mostly mechanical conversion once we decide what to convert it to.

Comment 4

[Jorg K (CEST = GMT+2)]

M-C suppresses warnings, see here:
https://hg.mozilla.org/mozilla-central/rev/f424e29dc77e#l3.12
we should do the same.

> There is bug 857230 for it. It
> is a large patch/project but mostly mechanical conversion once we decide
> what to convert it to.
There is no leeway for a decision here. We *must* convert before M-C remove it completely.

Comment 5

[:aceman]

The question is whether that is "before m-c remove it completely" OR "now, because it is already marked deprecated and causes build failure".

I have cleaned out nsIEnumerator in the past so that is done.
I do not see us referencing nsICollection anywhere either.

We use nsISupportsArray which may include nsICollection internally. I do not yet see why ImportOutFile.cpp fails, as it does not reference either one directly. May be due to the inclusion of nsMsgUtils.cpp which does include nsISupportsArray.

c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(38): warning C4996: 'nsICollection': was declared deprecated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(27): note: see declaration of 'nsICollection'
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(66): warning C4996: 'nsISupportsArray': was declared deprecated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(38): note: see declaration of 'nsISupportsArray'
mozmake.exe[5]: *** [ImportOutFile.obj] Error 2
mozmake.exe[4]: *** [mailnews/import/src/target] Error 2

Comment 6

[Jorg K (CEST = GMT+2)]

Is there more to it than wrapping all
https://dxr.mozilla.org/comm-central/search?q=nsISupportsArray.h&redirect=false
into this:

https://hg.mozilla.org/mozilla-central/rev/f424e29dc77e#l3.12

+// Disable deprecation warnings generated by nsISupportsArray and associated
+// classes.
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#elif defined(_MSC_VER)
+#pragma warning (push)
+#pragma warning (disable : 4996)
+#endif
+
 #include "nsSupportsArray.h"
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#elif defined(_MSC_VER)
+#pragma warning (pop)
+#endif
+

Comment 7

[Jorg K (CEST = GMT+2)]

I won't have time this afternoon. The person trying this approach, please register here so we don't waste time with two people addressing this at the same time.

Comment 9

[Jorg K (CEST = GMT+2)]

OK, I'll do it I have 30 minutes to edit 11 files ;-)

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1317040.patch

Try run here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=147da4f6414c3850d57e0e1ce9fd1a43db1f2c8b

I suggest that anyone with Level 3 rights can land this if the try turns out green. It's "pre-approved" with rs=bustage-fix ;-)

My 30 minutes are up ;-)

Comment 11

[:aceman]

I cannot work on this until tomorrow evening.
But I can promise to give review here if anybody makes a patch.

I'm fine if you choose to just hide the warnings, if it is only 11 files.

We will still need to get traction in bug 857230, but this temp solution will win us time to do it after 52. I'm willing to work on that one, after there is agreement on the API changes.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8810079 [details] [diff] [review]
1317040.patch

;-)

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8810079 [details] [diff] [review]
1317040.patch

Didn't work :-(

Comment 14

[Jorg K (CEST = GMT+2)]

Maybe wrap this as well:
mailnews/base/util/nsMsgUtils.h
38 class nsISupportsArray;
(I'm not at my desk.)

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #14)
> Maybe wrap this as well:
> mailnews/base/util/nsMsgUtils.h
> 38 class nsISupportsArray;
> (I'm not at my desk.)

Wrapped it there. Let's see if this helps.

https://hg.mozilla.org/try-comm-central/rev/91947445af7ed08842207b83933817902c1f1147

Comment 16

[Jorg K (CEST = GMT+2)]

I cancelled that. Let me try something else now.

Comment 17

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f64b2d92e7971d0eab071e122195d0f2f8ff0a4f

I wrapped the whole file from the include to the end. That should work ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

Actually, it's not 11 files that need fixing but 30:

mailnews/base/search/public/nsMsgSearchAdapter.h
mailnews/base/search/public/nsMsgSearchScopeTerm.h
mailnews/base/search/src/nsMsgFilter.cpp
mailnews/base/search/src/nsMsgFilter.h
mailnews/base/search/src/nsMsgFilterList.cpp
mailnews/base/search/src/nsMsgFilterService.cpp
mailnews/base/search/src/nsMsgImapSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.h
mailnews/base/search/src/nsMsgSearchAdapter.cpp
mailnews/base/search/src/nsMsgSearchImap.h
mailnews/base/search/src/nsMsgSearchNews.cpp
mailnews/base/search/src/nsMsgSearchNews.h
mailnews/base/search/src/nsMsgSearchSession.cpp
mailnews/base/search/src/nsMsgSearchSession.h
mailnews/base/search/src/nsMsgSearchTerm.cpp
mailnews/base/src/nsMsgAccountManager.cpp
mailnews/base/src/nsMsgFolderDataSource.cpp
mailnews/base/src/nsMsgFolderDataSource.h
mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
mailnews/base/src/nsSubscribeDataSource.cpp
mailnews/base/src/virtualFolderWrapper.js
mailnews/base/test/unit/test_iteratorUtils.js
mailnews/base/util/iteratorUtils.jsm
mailnews/base/util/nsMsgIncomingServer.cpp
mailnews/base/util/nsMsgUtils.cpp
mailnews/base/util/nsMsgUtils.h
mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp
mailnews/extensions/mailviews/src/nsMsgMailViewList.h
mailnews/imap/src/nsImapMailFolder.cpp

Comment 19

[Jorg K (CEST = GMT+2)]

Oops, need to remove the .js* from the list:

mailnews/base/search/public/nsMsgSearchAdapter.h
mailnews/base/search/public/nsMsgSearchScopeTerm.h
mailnews/base/search/src/nsMsgFilter.cpp
mailnews/base/search/src/nsMsgFilter.h
mailnews/base/search/src/nsMsgFilterList.cpp
mailnews/base/search/src/nsMsgFilterService.cpp
mailnews/base/search/src/nsMsgImapSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.h
mailnews/base/search/src/nsMsgSearchAdapter.cpp
mailnews/base/search/src/nsMsgSearchImap.h
mailnews/base/search/src/nsMsgSearchNews.cpp
mailnews/base/search/src/nsMsgSearchNews.h
mailnews/base/search/src/nsMsgSearchSession.cpp
mailnews/base/search/src/nsMsgSearchSession.h
mailnews/base/search/src/nsMsgSearchTerm.cpp
mailnews/base/src/nsMsgAccountManager.cpp
mailnews/base/src/nsMsgFolderDataSource.cpp
mailnews/base/src/nsMsgFolderDataSource.h
mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
mailnews/base/src/nsSubscribeDataSource.cpp
mailnews/base/util/nsMsgIncomingServer.cpp
mailnews/base/util/nsMsgUtils.cpp
mailnews/base/util/nsMsgUtils.h
mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp
mailnews/extensions/mailviews/src/nsMsgMailViewList.h
mailnews/imap/src/nsImapMailFolder.cpp

but need to add these:

mailnews/base/search/public/nsIMsgFilter.idl
mailnews/base/search/public/nsIMsgSearchSession.idl
mailnews/base/search/public/nsIMsgSearchValidityTable.idl
mailnews/extensions/mailviews/public/nsIMsgMailView.idl

Comment 20

[Jorg K (CEST = GMT+2)]

This is a bottom-less pit.

nsISupportsArray.h is included directly by 11 files.
nsIMsgFilter.h included directly by 10 files.
nsIMsgSearchSession.h is included directly by 19 files.
nsIMsgSearchValidityTable.h is included directly by 1 file.
nsIMsgMailView.h is not included anywhere but
nsIMsgMailView.idl is included in nsIMsgMailViewList.idl and
nsIMsgMailViewList.h is included in 1 file.

So far I've only done the first 11 files, and they need to be done differently, the pop at the end needs to go otherwise any of the files which include any of the header files amongst the 11 file will need the same treatment since the "warning disable" was popped.

Nice job two days before the branch day :-(

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1317040.patch (v2).

OK, fixing 11 .h and .cpp files which include nsISupportsArray.h and also 4 .idl files which include nsISupportsArray.idl or use interface nsISupportsArray.

No pop at the end so the warning will be disabled to the end of the respective files or any file that includes them or any .h generated by the .idl files.

See how that goes:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4f8e6ae960be3bdc23814e61b92b9429947ce465

Comment 22

[Jorg K (CEST = GMT+2)]

This still didn't work since the IDL processor turns this:
%{C++
#if defined(__GNUC__)
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#elif defined(_MSC_VER)
#pragma warning (disable : 4996)
#endif
%}
#include "nsISupportsArray.idl"


into this:
#ifndef __gen_nsISupportsArray_h__
#include "nsISupportsArray.h"
#endif

/* For IDL files that don't want to include root IDL files. */
#ifndef NS_NO_VTABLE
#define NS_NO_VTABLE
#endif
#if defined(__GNUC__)
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#elif defined(_MSC_VER)
#pragma warning (disable : 4996)
#endif

turning the order around. Grrrrrr.

Looks like I can't win at 3 AM.

Comment 23

[Jorg K (CEST = GMT+2)]

Another variation:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49338bc87ba99d9d97e5e887355f30235de135a8

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1317040.patch (v3).

This worked.

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/330fade4b94b549426eed14f6f9b44f65b7ef858

Comment 26

[:aceman]

Comment on attachment 8810155 [details] [diff] [review]
1317040.patch (v3).

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

Thanks, looks good for now.
But it will mask other depreciations in the future, so we should get rid of these exceptions soon (in the other bug).

Comment 27

[Jorg K (CEST = GMT+2)]

I love post landing reviews especially when they are positive ;-)

You are right this will mask other depreciations. The M-C approach I've seen was

warning push
warning disable
... some code
warning pop.

I tried to adopt this in a hurry, but it failed. Surely, if you do this in a .h file, all the .cpp which include the .h will NOT have the warning disabled. So this approach potentially only makes sense in .cpp files. There however, it also doesn't make sense. Why should I pop the warning at the end of the source file? Maybe to cater for multiple source files to be glued together to a compile unit? Maybe that's why M-C do it.

The patch treated 15 files: 4 .idl, 4 .h and 7 .cpp. If you want, I can see whether I can improve the .cpp files based on the M-C model described above. Most likely for little benefit.

What this bug has taught us is that we are in deep trouble as far as nsISupportsArray is concerned. It's all over the place, see comment #19, and the day it's removed from M-C we're busted big time.