1481326 - Port bug 1090497 - Re-enable warnings as errors on clang-cl to mailnews

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Attachment: clang-cl warnings-comm.txt

We already know that clang-cl is particularly picky.

After the last M-C merge we see:
fatal error: too many errors emitted, stopping now [-ferror-limit=]
mozmake.EXE[4]: *** [nsComposeStrings.obj] Error 1
mozmake.EXE[3]: *** [comm/mailnews/compose/src/target] Error 2
mozmake.EXE[4]: *** [nsMessengerWinIntegration.obj] Error 1
mozmake.EXE[4]: *** [nsAbOutlookDirFactory.obj] Error 1
mozmake.EXE[4]: *** [nsAbOutlookDirectory.obj] Error 1
mozmake.EXE[4]: *** [nsAbWinHelper.obj] Error 1
mozmake.EXE[4]: *** [nsMapiAddressBook.obj] Error 1
mozmake.EXE[3]: *** [comm/mailnews/base/src/target] Error 2
mozmake.EXE[3]: *** [comm/mailnews/addrbook/src/target] Error 2

In the attachment I've collected all the clang-cl warning we have in comm/, a few, for example in libical and ldap-sdk are already ignored, see use of AllowCompilerWarnings().

Now we'll need to add
+if CONFIG['CC_TYPE'] == 'clang-cl':
+    AllowCompilerWarnings()  # workaround for bug 1090497/xxx (this one here)
to a few more places ... or fix some of the warnings. Easy wins are unused variables.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1481326-clang-cl-warnings.patch - WIP (v1)

Here are some changes, far from complete.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1481326-clang-cl-warnings.patch (v2)

I did what I could, fixing obvious errors and allowing the remaining warnings. This compiles locally on MSVS C++, haha.

Let's see how far we get:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9e77504a0ecda4fb2eb4bcb91c59baa872e13e32

Comment 3

[Jorg K (CEST = GMT+2)]

Missed one, this one should be OK:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7a6bca736bfa94e019ee55ef1f326adc686aff53

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1481326-clang-cl-warnings.patch (v2b)

Comment 5

[Jorg K (CEST = GMT+2)]

I forgot to say: nsOEProfileMigrator.cpp/h was a left-over from bug 1338715.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16a4b760e352
fix some clang-cl warnings and allow others. rs=bustage-fix

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: remaining-warnings.txt

Heaps of warnings are left, 367 to be precise, you need to check the log of a *successful* compile to see them.

For example from the try run:
https://taskcluster-artifacts.net/D0kfR57FRnOGW2PA1aiU9w/0/public/logs/live_backing.log

The most prominent ones are:
db/mork/src/morkObject.h(96,26):  warning: 'morkObject::AddStrongRef' hides overloaded virtual function [-Woverloaded-virtual]
mail/components/shell/nsMailWinIntegration.cpp(84,5):  warning: ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]
mailnews/compose/src/nsComposeStrings.cpp(64,10):  warning: case value not in enumerated type 'nsresult' [-Wswitch]

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: more.patch -one more unused variable

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f67c130b8d32
Follow-up: Remove another unused variable. rs=bustage-fix DONTBUILD

Comment 10

[:aceman]

Comment on attachment 8998098 [details] [diff] [review]
1481326-clang-cl-warnings.patch (v2)

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

Is the Outlook Express migrator dead?

Thanks for the fixes. I see you already landed this.
Can you fix the problems I found in a patch on top of this?

::: mail/components/search/wsenable/moz.build
@@ +19,5 @@
>  # This isn't XPCOM code, but it wants to use STL so disable STL wrappers
>  DisableStlWrapping()
> +
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

Can't we rather fix this WSEnable.cpp ? What is the warning about? What do the special L"" strings mean?

::: mail/components/shell/nsMailWinIntegration.cpp
@@ -209,5 @@
>    // RSS / feed protocol shell integration is not working so return true
>    // until it is fixed (bug 445823).
>    if (aApps & nsIShellService::RSS)
>      *aIsDefaultClient &= true;
> -//    *aIsDefaultClient &= TestForDefault(gFeedSettings, sizeof(gFeedSettings)/sizeof(SETTING));

I think there was just some bug with getting the defaults for RSS, that is why it is disabled.

I wouldn't remove all this, just comment out gFeedSettings too.

::: mailnews/addrbook/src/nsAbOutlookDirFactory.cpp
@@ -15,5 @@
>  #include "mozilla/Logging.h"
>  
> -static mozilla::LazyLogModule gAbOutlookDirFactoryLog("AbOutlookDirFactory");
> -
> -#define PRINTF(args) MOZ_LOG(gAbOutlookDirFactoryLog, mozilla::LogLevel::Debug, args)

Nothing is logged in this file? Can #include "mozilla/Logging.h" go too?

::: mailnews/addrbook/src/nsAbWinHelper.cpp
@@ -960,5 @@
>  const int kOutlookStubLength = 3 ;
>  const char *kOutlookExpStub = "oe/" ;
>  const int kOutlookExpStubLength = 3 ;
>  const char *kOutlookCardScheme = "moz-aboutlookcard://" ;
> -const int kOutlookCardSchemeLength = 16 ;

Well, all the other constants have a matching *Length constant. This one is unused? Can the same value be retrieved using e.g. strlen(kOutlookCardScheme) if needed?

::: mailnews/base/src/nsMsgSearchDBView.cpp
@@ +286,5 @@
>                                       nsIDBChangeListener *aInstigator)
>  {
>    // Defer to base class if we're grouped or not threaded at all.
>    if (m_viewFlags & nsMsgViewFlagsType::kGroupBySort ||
> +      !(m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay))

Nice one.

::: mailnews/compose/src/moz.build
@@ +57,5 @@
>  
>  FINAL_LIBRARY = 'mail'
>  
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

Can we instead mark the individual switches causing the "case value not in enumerated type 'nsresult'" instead of ignoring the warnings for whole compose folder?

::: mailnews/imap/src/nsImapUtils.cpp
@@ +174,5 @@
>  }
>  
>  NS_IMETHODIMP nsImapMailboxSpec::GetUnicharPathName(nsAString &aUnicharPathName)
>  {
> +  aUnicharPathName = mUnicharPathName;

Nice one. I wonder how this ever produced a useful return value that worked for the caller.

::: mailnews/import/outlook/src/moz.build
@@ +22,5 @@
>      '../../src'
>  ]
>  
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

Is this for mailnews/import/outlook/src/rtfDecoder.cpp ? Can't those warnings be fixed by adding them to the switch, or having a "default" fallback?

::: mailnews/import/winlivemail/moz.build
@@ +12,5 @@
>  
>  FINAL_LIBRARY = 'import'
> +
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

Is "mailnews/import/winlivemail/nsWMStringBundle.cpp(23,24):  warning: ISO C++11 does not allow conversion from string literal to 'char *'" unfixable?

::: mailnews/mapi/mapiDll/moz.build
@@ +21,5 @@
>  
>  DEFFILE = SRCDIR + '/Mapi32.def'
> +
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

Are the 3 warnings unfixable?

::: mailnews/mapi/mapihook/src/moz.build
@@ +22,5 @@
>  DEFINES['UNICODE'] = True
>  DEFINES['_UNICODE'] = True
> +
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    AllowCompilerWarnings()  # workaround for bug 1090497 / bug 1481326

At least "mailnews/mapi/mapihook/src/msgMapiMain.h(5,9):  warning: 'MSG_MAPI_MAIN_H_' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]" looks fixable and is a real bug.

Comment 11

[:aceman]

Comment on attachment 8998116 [details] [diff] [review]
more.patch -one more unused variable

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

::: mailnews/mapi/mapihook/src/msgMapiImp.cpp
@@ +578,1 @@
>      if (msgHdr)

Or you could also add check of NS_SUCCEEDED(rv) here.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #10)
> Is the Outlook Express migrator dead?
Yes, as I said in comment #5, it was removed in bug 1338715.

> Thanks for the fixes. I see you already landed this.
> Can you fix the problems I found in a patch on top of this?
Hmm, I'm really hired to keep everything compiling and no one cared for 10+ years until yesterday ;-)

I had to add AllowCompilerWarnings() ten times. So instead of making this another 200-comment bug, we should open new bugs to clean up the issues per directory, or at least one new bug with ten parts to land when a patch is required.

Some stuff, like for example in mork appears to be unfixable easily and I'm surprised that none of the other platforms have yet complained which also use some form of clang, no?

> Can we instead mark the individual switches causing the "case value not in
> enumerated type 'nsresult'" instead of ignoring the warnings for whole compose folder?
Agreed. There is already: https://hg.mozilla.org/comm-central/rev/16a4b760e352#l28.9
However, M-C added the AllowCompilerWarnings() whole-sale, so I followed that.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks, I filed bug 1481417 as a follow-up.

Comment 14

[:aceman]

(In reply to Jorg K (GMT+2) from comment #12)
> Some stuff, like for example in mork appears to be unfixable easily and I'm
> surprised that none of the other platforms have yet complained which also
> use some form of clang, no?

Yes. Maybe when Windows now switched to clang, they use the newest clang version, while on the other platforms they keep some old version for now, like was the case with gcc.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/cd0576a4915bd793f24e5128bbebefe7042a6e86
fix some clang-cl warnings and allow others (MAPI part only). r=aceman a=jorgk