Closed Bug 1481417 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

Attachments

(5 files, 15 obsolete files)

793 bytes, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
758 bytes, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
6.56 KB, patch
jorgk-bmo
: review+
aceman
: feedback+
Details | Diff | Splinter Review
5.01 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
9.46 KB, patch
jorgk-bmo
: review+
aceman
: feedback+
Details | Diff | Splinter Review
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.
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
This bug could handsomely produce ten patches ;-) - Here's the first one.
Attachment #8998265 - Flags: review?(acelists)
Attached patch 1481417-part2-db-mork-src.patch (obsolete) — Splinter Review
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).
Attachment #8998269 - Flags: review?(acelists)
Fixed bad English.
Attachment #8998269 - Attachment is obsolete: true
Attachment #8998269 - Flags: review?(acelists)
Attachment #8998273 - Flags: review?(acelists)
Attachment #8998265 - Flags: review?(acelists) → review+
Attachment #8998273 - Flags: review?(acelists) → review+
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 :-(
Hmm, thanks for the review, but it doesn't work. Please read previous comment #5.
Aceman did all the work here, so let's attribute it to him.
Attachment #8998265 - Attachment is obsolete: true
Attachment #8998273 - Attachment is obsolete: true
Attachment #8998429 - Flags: review+
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.
Sure. I'll just get the first two landed.
Pushed by mozilla@jorgk.com:
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 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 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.
(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 :)
How about this?
Attachment #8998652 - Attachment is obsolete: true
Attachment #8998652 - Flags: review?(jorgk)
Attachment #8998660 - Flags: review+
Attachment #8998660 - Flags: feedback?(acelists)
Oops, killed a newline.
Attachment #8998660 - Attachment is obsolete: true
Attachment #8998660 - Flags: feedback?(acelists)
Attachment #8998661 - Flags: review+
Attachment #8998661 - Flags: feedback?(acelists)
Attachment #8998665 - Flags: review?(jorgk)
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.
Attachment #8998665 - Flags: review?(jorgk) → review+
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
Attachment #8998661 - Flags: feedback?(acelists) → feedback+
Attachment #8998665 - Attachment is obsolete: true
Attachment #8998676 - Flags: review+
Attachment #8998653 - Attachment is obsolete: true
Attachment #8998653 - Flags: review?(jorgk)
Comment on attachment 8998684 [details] [diff] [review]
1481417-5-mail-components.patch (v2)

Making the destructor virtual did the trick.
Attachment #8998684 - Flags: review+
Nice, thanks.
Pushed by mozilla@jorgk.com:
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
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.
Attached patch 1481417-part5-mapi.patch (obsolete) — Splinter Review
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
Attachment #8998726 - Attachment is obsolete: true
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.
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?
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.
Assignee: nobody → acelists
Keywords: leave-open
Attached patch 1481417-part5-mapi.patch (obsolete) — Splinter Review
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.
Attachment #8999094 - Attachment is obsolete: true
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"");
Attached patch 1481417-part5-mapi.patch (obsolete) — Splinter Review
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 :-(
Attachment #8999102 - Attachment is obsolete: true
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.
Pushed by mozilla@jorgk.com:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
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 on attachment 8999156 [details] [diff] [review]
1481417-part5-mapi.patch

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

Thanks.
Attachment #8999156 - Flags: feedback?(acelists) → feedback+
You need to log in before you can comment on or make changes to this bug.