1481417 - Investigate and fix clang-cl warnings on Windows (follow-up of bug 1481326)

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

In bug 1481326 we added AllowCompilerWarnings() ten times to not turn compiler warnings into errors.

We should investigate whether some of those can be removed and the warnings fixed instead.

Comment 1

[Jorg K (CEST = GMT+2)]

See bug 1481326 comment #7 and attachment 8998115 [details] which lists the remaining 367 errors. It would be good to remove some of the AllowCompilerWarnings() and either fix the warnings or add more specific exemptions like here:
https://hg.mozilla.org/comm-central/rev/16a4b760e352#l28.9

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part1-mailnews-compose-src.patch

This bug could handsomely produce ten patches ;-) - Here's the first one.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part2-db-mork-src.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c2252c8b589bd871ee208eb6a0c1b88e5aaa40f6 for both patches (you only see the second one since the first one was pushed before).

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part2-db-mork-src.patch (v1b)

Fixed bad English.

Comment 5

[Jorg K (CEST = GMT+2)]

Looking at the remaining eight directories:
1) mail/components/search
Not an easy win here.

2) mail/components/search/wsenable
Only: warning: ISO C++11 does not allow conversion from string literal to 'LPWSTR' (aka 'wchar_t *') [-Wwritable-strings]

3) mail/components/shell
Only: warning: ISO C++11 does not allow conversion from string literal to 'LPWSTR' (aka 'wchar_t *') [-Wwritable-strings]

4) mailnews/import/outlook/src
Not an easy win here.

5) mailnews/import/winlivemail
Only: warning: ISO C++11 does not allow conversion from string literal to 'LPWSTR' (aka 'wchar_t *') [-Wwritable-strings]

6) mailnews/mapi/mapiDll
warning: ISO C++11 does not allow conversion from string literal to 'LPWSTR' (aka 'wchar_t *') [-Wwritable-strings]
and one potentially uninitialised variable.

7) mailnews/mapi/mapihook/build
Not an easy win here.

8) mailnews/mapi/mapihook/src
Not an easy win here.

BTW, scrap the reviews, the try failed :-( - Somehow that syntax isn't right:
comm/mailnews/compose/src/nsComposeStrings.cpp(64,10):  error: case value not in enumerated type 'nsresult' [-Werror,-Wswitch]

So I was quite correct to whole-sale add AllowCompilerWarnings() as a quick bustage fix. We could be here for a while :-(

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, thanks for the review, but it doesn't work. Please read previous comment #5.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part1-mailnews-compose-src.patch (v2)

Aceman did all the work here, so let's attribute it to him.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part2-db-mork-src.patch (v2)

Comment 9

[:aceman]

Thanks. So I have a set of 5 patches on try now, that seem to work, except the LPTSTR in mailnews/mapi/mapiDll/MapiDll.cpp . But they are not ready, I have some more tweaks to them locally and will submit them in the evening.

Comment 10

[Jorg K (CEST = GMT+2)]

Sure. I'll just get the first two landed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c91a7822ca0
Part 1: Replace AllowCompilerWarnings() with -Wno-error=switch in mailnews/compose/src. r=jorgk
https://hg.mozilla.org/comm-central/rev/492146f1c570
Part 2: Replace AllowCompilerWarnings() with -Wno-error=overloaded-virtual in db/mork/src. r=jorgk DONTBUILD

Comment 12

[:aceman]

Attachment: 1481417-4-mailnews-import.patch

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

Comment 13

[:aceman]

Attachment: 1481417-5-mail-components.patch

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

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998652 [details] [diff] [review]
1481417-4-mailnews-import.patch

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

::: mailnews/import/outlook/src/MapiApi.cpp
@@ +1219,5 @@
>    if (PROP_TYPE(tag) == PT_UNICODE) // unicode string
>      LossyCopyUTF16toASCII(nsDependentString(static_cast<wchar_t*>(result)), val);
>    else // either PT_STRING8 or some other binary - use as is
>      val.Assign(static_cast<char*>(result));
> +  // Despite being used as wchar_t*, resuls it allocated as "new char[]" in GetLargeProperty().

typo: result. Sorry you copied 1:1 from IRC ;-(

@@ +1233,5 @@
>    if (PROP_TYPE(tag) == PT_UNICODE) // We already get the unicode string
>      val.Assign(static_cast<wchar_t*>(result));
>    else // either PT_STRING8 or some other binary
>      CStrToUnicode(static_cast<char*>(result), val);
> +  // Despite being used as wchar_t*, resuls it allocated as "new char[]" in GetLargeProperty().

And here.

::: mailnews/import/outlook/src/MapiMessage.h
@@ -93,1 @@
>    class CHeaderField {

So by removing 'private:' this class is now public. Can't you use "friend class" to let some other class access the private class? You can compile that locally to see whether it works.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998653 [details] [diff] [review]
1481417-5-mail-components.patch

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

::: mail/components/search/moz.build
@@ +24,2 @@
>  if CONFIG['CC_TYPE'] == 'clang-cl':
> +    CXXFLAGS += ['-Wno-error=delete-non-virtual-dtor']

That's from:
mail/components/search/nsMailWinSearchHelper.cpp(46,1):  warning: delete called on non-final 'nsMailWinSearchHelper' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor]
Is that not fixable?

::: mail/components/search/nsMailWinSearchHelper.cpp
@@ +203,2 @@
>      pAAR->QueryAppIsDefault(L".wdseml", AT_FILEEXTENSION, AL_EFFECTIVE, APP_REG_NAME_MAIL, &res);
> +    *aResult = res;

This change isn't right. The function always returns NS_OK, so it should always set aResult. So BOOL res = false; will fix that, no further reshuffling required.

@@ -257,5 @@
>    executeInfo.lpFile = filePath.get();
>    executeInfo.lpParameters = params.get();
>    executeInfo.nShow = SW_SHOWNORMAL;
>  
> -  DWORD dwRet;

Hmm, a lot of reshuffling here when all that is required is 
= ERROR_SUCCESS; here.

BTW, ERROR_SUCCESS is 0x0, so assuming the local variable was nulled, we don't change any behaviour.

Comment 16

[:aceman]

(In reply to Jorg K (GMT+2) from comment #15)
> ::: mail/components/search/moz.build
> @@ +24,2 @@
> >  if CONFIG['CC_TYPE'] == 'clang-cl':
> > +    CXXFLAGS += ['-Wno-error=delete-non-virtual-dtor']
> 
> That's from:
> mail/components/search/nsMailWinSearchHelper.cpp(46,1):  warning: delete
> called on non-final 'nsMailWinSearchHelper' that has virtual functions but
> non-virtual destructor [-Wdelete-non-virtual-dtor]
> Is that not fixable?

Maybe, but I don't know what to do there. The warning will stay visible for someone to fix.
 
> ::: mail/components/search/nsMailWinSearchHelper.cpp
> @@ +203,2 @@
> >      pAAR->QueryAppIsDefault(L".wdseml", AT_FILEEXTENSION, AL_EFFECTIVE, APP_REG_NAME_MAIL, &res);
> > +    *aResult = res;
> 
> This change isn't right. The function always returns NS_OK, so it should
> always set aResult. So BOOL res = false; will fix that, no further
> reshuffling required.

aResult is initialized to false at the top of the function. But I can do what you say.
 
> @@ -257,5 @@
> >    executeInfo.lpFile = filePath.get();
> >    executeInfo.lpParameters = params.get();
> >    executeInfo.nShow = SW_SHOWNORMAL;
> >  
> > -  DWORD dwRet;
> 
> Hmm, a lot of reshuffling here when all that is required is 
> = ERROR_SUCCESS; here.
> 
> BTW, ERROR_SUCCESS is 0x0, so assuming the local variable was nulled, we
> don't change any behaviour.

The reshuffling made the code cleaner with less indent, but sure, I can put it back :)

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-4-mailnews-import.patch - part 3

How about this?

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-4-mailnews-import.patch - part 3

Oops, killed a newline.

Comment 19

[:aceman]

Attachment: 1481417-5-mail-components.patch v1.1

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998665 [details] [diff] [review]
1481417-5-mail-components.patch v1.1

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

Thanks, less reshuffling.

::: mail/components/search/moz.build
@@ +19,5 @@
>  ]
>  
>  JAR_MANIFESTS += ['jar.mn']
>  
> +# clang-cl rightly complains here.

Rightly or now, we don't know. So make it:
# clang-cl complains about this.
like I have in the other patch.

Comment 21

[:aceman]

Comment on attachment 8998661 [details] [diff] [review]
1481417-4-mailnews-import.patch - part 3

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

Thanks, works for me on Linux/gcc too.
And try run on Windows/clang: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=445b11d1d9b893ec3418d77e722ed5a9245fc428

Comment 22

[:aceman]

Attachment: 1481417-5-mail-components.patch v1.2

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-5-mail-components.patch (v2)

Let's see whether we can get rid of delete-non-virtual-dtor:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=289d6dd1f3ef48ac10da560cb446fe8aa6b6516c

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998684 [details] [diff] [review]
1481417-5-mail-components.patch (v2)

Making the destructor virtual did the trick.

Comment 25

[:aceman]

Nice, thanks.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1853bb5a2acf
Part 3: Fix warnings and replace AllowCompilerWarnings() with -Wno-error=switch in mailnews/import/. r=jorgk
https://hg.mozilla.org/comm-central/rev/6483ed734746
Part 4: Fix warnings and remove AllowCompilerWarnings() in mail/components/. r=jorgk DONTBUILD

Comment 27

[Jorg K (CEST = GMT+2)]

Yes, we used virtual DTORs in various other places.

We're 70% done with these left:
- mailnews/mapi/mapiDll
- mailnews/mapi/mapihook/build
- mailnews/mapi/mapihook/src
Note Aceman's WIP: https://hg.mozilla.org/try-comm-central/rev/25b9577e4f57504841eee316d2f747838e80c7c7
These are a little harder since there are errors in generated files, like mailnews/mapi/mapihook/build/dlldata.c and mailnews/mapi/mapihook/build/msgMapi_p.c.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e471f0c4ce340f071f56d4b4c0af4ef28f5ec0f3

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

Oops, needed to do the (? : ) twice:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d041b21d3797bfe0534a487e89a534cb24c4c3b8
Is that known to be (not) working?

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

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

The (? : ) worked, but there was a 'const' missing.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

The problem remaining is:
INFO -  z:/build/build/src/comm/mailnews/mapi/mapiDLL/MapiDll.cpp(295,28):  error: cannot initialize a parameter of type 'LPTSTR' (aka 'wchar_t *') with an rvalue of type 'const TCHAR *' (aka 'const wchar_t *')
INFO -                             (lpszMessageType ? lpszMessageType : L""),
INFO -                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
INFO -  z:/build/build/src/obj-thunderbird/dist/include\msgMapi.h(164,39):  note: passing argument to parameter 'lpszMessageType' here
INFO -              /* [unique][in] */ LPTSTR lpszMessageType,
INFO -                                        ^
INFO -  1 error generated.

Looks like "const" is missing somehow in msgMapi.idl. This is MS IDL territory, so I added it according to
https://docs.microsoft.com/en-us/windows/desktop/midl/const

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8f9622965f906b150efaf3e303fb5a13bcb53a2f

Comment 32

[Jorg K (CEST = GMT+2)]

Moved the const, went to the wrong patch, but never mind:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d3982db9788b55b73a8c21bf59816bb063aedc9

Comment 33

[:aceman]

Comment on attachment 8998740 [details] [diff] [review]
1481417-part5-mapi.patch

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

::: mailnews/mapi/mapiDll/MapiDll.cpp
@@ +292,5 @@
>      return MAPI_E_INVALID_SESSION;
>  
> +  return pNsMapi->FindNext(lhSession, ulUIParam,
> +                           (lpszMessageType ? lpszMessageType : L""),
> +                           (lpszSeedMessageID ? lpszSeedMessageID : L""),

I already tried exactly this :) But is then the 'const' needed in MAPIFindNext? Adding 'const' to FindNext is interesting, I haven't tried that.

::: mailnews/mapi/mapihook/build/msgMapi.idl
@@ +71,5 @@
>      HRESULT SendDocuments([in] unsigned long aSession,
>                            [in, unique] LPTSTR aDelimChar, [in, unique] LPTSTR aFilePaths,
>                            [in, unique] LPTSTR aFileNames, [in] ULONG aFlags ) ;
>  
> +    HRESULT FindNext([in] unsigned long aSession, [in] ULONG ulUIParam, [in, unique] const LPTSTR lpszMessageType,

Nice try.

Comment 34

[Jorg K (CEST = GMT+2)]

The last try was:
HRESULT FindNext([in] unsigned long aSession, [in] ULONG ulUIParam, [in, unique] LPTSTR const lpszMessageType,

Where does the MAPIFindNext function come from?

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

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

Comment 36

[Jorg K (CEST = GMT+2)]

Aceman wanted two errors, here they are ;-)
INFO -  z:/build/build/src/comm/mailnews/mapi/mapiDLL/MapiDll.cpp(294,16):  error: cannot initialize a variable of type 'const LPTSTR' (aka 'wchar_t *const') with an rvalue of type 'const TCHAR *' (aka 'const wchar_t *')
INFO -    const LPTSTR type = lpszMessageType ? lpszMessageType : L"";
INFO -                 ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
INFO -  z:/build/build/src/comm/mailnews/mapi/mapiDLL/MapiDll.cpp(295,16):  error: cannot initialize a variable of type 'const LPTSTR' (aka 'wchar_t *const') with an rvalue of type 'const TCHAR *' (aka 'const wchar_t *')
INFO -    const LPTSTR id = lpszSeedMessageID ? lpszSeedMessageID : L"";
INFO -                 ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
INFO -  2 errors generated.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

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

INFO -  z:/build/build/src/comm/mailnews/mapi/mapiDLL/MapiDll.cpp(294,59):  error: static_cast from 'const wchar_t *' to 'const LPTSTR' (aka 'wchar_t *const') is not allowed
INFO -    const LPTSTR type = lpszMessageType ? lpszMessageType : static_cast<const LPTSTR>(L"");
INFO -                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
INFO -  z:/build/build/src/comm/mailnews/mapi/mapiDLL/MapiDll.cpp(295,61):  error: static_cast from 'const wchar_t *' to 'const LPTSTR' (aka 'wchar_t *const') is not allowed
INFO -    const LPTSTR id = lpszSeedMessageID ? lpszSeedMessageID : static_cast<const LPTSTR>(L"");
INFO -                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
INFO -  2 errors generated.

Comment 38

[Jorg K (CEST = GMT+2)]

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

Trying a C-style cast:
+  const LPTSTR type = lpszMessageType ? lpszMessageType : (const LPTSTR)(L"");
+  const LPTSTR id = lpszSeedMessageID ? lpszSeedMessageID : (const LPTSTR)(L"");

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

That actually compiled with the C-style case. But there are more errors:

08:36:29     INFO -  z:/build/build/src/obj-thunderbird/comm/mailnews/mapi/mapihook/build/dlldata.c(24,1):  error: 'extern' variable has an initializer [-Werror,-Wextern-initializer]
08:36:29     INFO -  PROXYFILE_LIST_START

comm/mailnews/mapi/mapihook/src/msgMapiImp.cpp(83,9):  error: delete called on non-final 'CMapiImp' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
08:36:35     INFO -          delete this;

I thought I had the first one covered with
+    CXXFLAGS += ['-Wno-error=extern-initializer','-Wno-error=missing-braces','-Wno-error=unused-const-variable']

No! This is C code :-(

Comment 40

[:aceman]

I thought we can leave the generated files like dlldata.c with all warnings allowed. If they are generated by third party code, the warnings may change in the future. If we can't fix them, we probably do not care what they are. Similar to how we ignore the LDAP lib.

But you may need CFLAGS for that file.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: 1481417-part5-mapi.patch

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

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16dc2cb0e535
Part 5: Fix warnings and remove AllowCompilerWarnings() or replace it with something more specific in mailnews/mapi. r=jorgk

Comment 43

[Jorg K (CEST = GMT+2)]

I needed a patch, so I landed this one, make if PLF ;-)

(In reply to :aceman from comment #40)
> I thought we can leave the generated files like dlldata.c with all warnings
> allowed. If they are generated by third party code, the warnings may change
> in the future. If we can't fix them, we probably do not care what they are.
> Similar to how we ignore the LDAP lib.
I don't agree. Either we care about warnings or we don't. Since we do, we should also look at the warnings in third-party code while we're here. The "allow warnings" was added in a hurry in bug 1294260.

libical is a sad case, with a plethora of warnings, so listing them individually, doesn't really improve the situation.

ldap/c-sdk isn't quite as bad, we could list these instead:
macro-redefined, format, int-to-void-pointer-cast, unused-variable, incompatible-pointer-types, dangling-else.
I would even go as far as to actually fix everything but the "macro-redefined".

Comment 44

[:aceman]

Comment on attachment 8999156 [details] [diff] [review]
1481417-part5-mapi.patch

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

Thanks.