1198936 - VS2015 build fails in mailnews due to wchar_t/char16_t mismatch

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Stefan Sitter [:ssitter]]

When trying to compile Thunderbird using Microsoft Visual Studio 2015 (VC14) with Windows 8.1 SDK on Windows 7 the following errors break compilation:

> [...]/mailnews/import/outlook/src/MapiApi.cpp(299): 
> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

> [...]/mailnews/import/outlook/src/MapiApi.cpp(307): 
> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

> [...]/mailnews/import/outlook/src/MapiApi.cpp(997): 
> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

> [...]/mailnews/import/outlook/src/MapiMessage.cpp(603): 
> error C2664: 'nsresult nsIUnicodeEncoder::Convert(const char16_t *,int32_t *,char *,int32_t *)': cannot convert argument 1 from 'const wchar_t *' to 'const char16_t *'

> [...]/mailnews/import/outlook/src/MapiMessage.cpp(650): 
> error C2665: 'Substring': none of the 8 overloads could convert all the argument types

> [...]/mailnews/import/oexpress/WabObject.cpp(525): 
> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

> [...]/mailnews/import/oexpress/WabObject.cpp(533): 
> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

Comment 1

[Stefan Sitter [:ssitter]]

Some other errors that are reported regarding wchar_t / char16_t mismatch:

> mailnews/base/util/nsMsgUtils.cpp(306): error C2440: 'reinterpret_cast': cannot convert from 'char16ptr_t' to 'const char *'
> mailnews/base/util/nsMsgUtils.cpp(306): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
> 
> mailnews/base/util/nsMsgI18N.cpp(115): error C2666: 'char16ptr_t::operator []': 3 overloads have similar conversions
> obj-i686-pc-mingw32\dist\include\mozilla/Char16.h(155): note: could be 'char16_t char16ptr_t::operator [](size_t) const'
> mailnews/base/util/nsMsgI18N.cpp(115): note: while trying to match the argument list '(char16ptr_t, int)'
> 
> mailnews/addrbook/src/nsAbWinHelper.cpp(520): error C2440: 'const_cast': cannot convert from 'const char16_t *' to 'WCHAR *'
> mailnews/addrbook/src/nsAbWinHelper.cpp(520): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
> 
> mailnews/addrbook/src/nsAbWinHelper.cpp(548): error C2440: 'const_cast': cannot convert from 'char16ptr_t' to 'WCHAR *'
> mailnews/addrbook/src/nsAbWinHelper.cpp(548): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
> 
> mailnews/addrbook/src/nsAbWinHelper.cpp(637): error C2440: 'const_cast': cannot convert from 'char16ptr_t' to 'WCHAR *'
> mailnews/addrbook/src/nsAbWinHelper.cpp(637): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
> 
> mailnews/addrbook/src/nsAbWinHelper.cpp(700): error C2440: 'const_cast': cannot convert from 'char16ptr_t' to 'WCHAR *'
> mailnews/addrbook/src/nsAbWinHelper.cpp(700): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
> 
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): error C2593: 'operator *' is ambiguous
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: could be 'built-in C++ operator*(const char16_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(const wchar_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(char16_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(wchar_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(const char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(const unsigned char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: or       'built-in C++ operator*(unsigned char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1309): note: while trying to match the argument list '(char16ptr_t)'
> 
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): error C2593: 'operator *' is ambiguous
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: could be 'built-in C++ operator*(const char16_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(const wchar_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(char16_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(wchar_t *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(const char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(const unsigned char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: or       'built-in C++ operator*(unsigned char *)'
> mailnews/addrbook/src/nsAbOutlookDirectory.cpp(1323): note: while trying to match the argument list '(char16ptr_t)'
> 
> mailnews/mapi/mapihook/src/msgMapiImp.cpp(173): error C2664: 'int16_t nsMAPIConfiguration::RegisterSession(uint32_t,const char16_t *,const char16_t *,bool,bool,uint32_t *,const char *)': cannot convert argument 2 from 'wchar_t []' to 'const char16_t *'
> mailnews/mapi/mapihook/src/msgMapiImp.cpp(173): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Comment 2

[Philip Chee]

> error C2664: 'int MultiByteToWideChar(UINT,DWORD,LPCCH,int,LPWSTR,int)': cannot convert argument 5 from 'char16_t *' to 'LPWSTR'

Similar bug.
Bug 1187201 - Char16 wrapper for VS2015 doesn't work with external NS_ConvertUTF16toUTF8

Comment 3

[Stefan Sitter [:ssitter]]

Unfortunately those errors are only the beginning. I fixed some just to get the next errors that are caused by mixing up wchar_t and char16_t. Neil mentioned in mozilla.dev.apps.seamonkey that we can use wwc() in some places to cast between both. But in other places we need to decide what type should be used. Are wchar_t and char16_t considered identical in Mozilla code? Is there one type preferred over the other?

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: VS2015 Build fix for import directory

I am not much of a C++ programmer so I wouldn't call it a patch yet. Build goes further with it but unable to test because no Outlook installed.

Comment 5

[Stefan Sitter [:ssitter]]

Attachment: fix wchar_t/char16_t mismatch in mail and mailnews

This is my current WIP patch. Together with patch from Bug 1200039 the build continues until it tries to link / package non-existing VC++ runtime DLLs.

Comment 6

[Frank-Rainer Grahl (:frg)]

Do you know what 
   if (*properties[index_DisplayName].get() == 0) 

in mailnews/addrbook/src/nsAbOutlookDirectory.cpp is doing? Is 0 right or is it supposed to check for a nullptr? I can't make much sense of it but I am eager to learn :)

Comment 7

[Stefan Sitter [:ssitter]]

Based on the surrounding code I assumed the code tries to detect an empty string. They get the pointer to the the null-terminated string, dereference the pointer to get the first character and check if this character is 0 (the null termination character). I think it could be written as | if (displayName[0] == L'\0') | or maybe just | if ( properties[index_DisplayName].IsEmpty() ) |.

Comment 8

[Frank-Rainer Grahl (:frg)]

Thanks for the clarification. The 0 without quotes just didn't look right in my eyes. 

I was able to compile an installation package for Seamonkey without the runtime DLLs (I have them installed anyway). Link worked. Needed another change in the suite directory which I haven't reported yet but it seems to run ok.

Comment 9

[[e-mail]]

Comment on attachment 8654652 [details] [diff] [review]
fix wchar_t/char16_t mismatch in mail and mailnews

Drive-by nits:

>-  char16_t appPath[MAX_BUF];
>+  wchar_t appPath[MAX_BUF];
[Why not use WCHAR since this is the Windows name for it?]

>-  if (*properties[index_DisplayName].get() == 0) {
This code sucks. What it actually wants to say is
if (properties[index_DisplayName].IsEmpty()) {

>-        value.Value.lpszW = const_cast<WCHAR *>(aValue) ;
>+        value.Value.lpszW = wwc(const_cast<char16_t *>(aValue)) ;
Did you try wwc(aValue)?

>-    displayName.Value.lpszW = const_cast<WCHAR *>(tempName.get()) ;
>+    const wchar_t *tempNameValue = tempName.get();
>+    displayName.Value.lpszW = const_cast<wchar_t *>(tempNameValue) ;
Try (wchar_t*)tempName.get()

>-      if (!tmp.IsEmpty() && tmp.get()[0] == char16_t(0xFEFF))
>-        tmp.Cut(0, 1);
Use tmp.First() instead of tmp.get()[0]

>-    return StringHash(reinterpret_cast<const char*>(str.get()),
>+    const char16_t *strbuf = str.get();
>+    return StringHash(reinterpret_cast<const char*>(strbuf),
>                       str.Length() * 2);
Try (const char *)str.get()

>-    LossyCopyUTF16toASCII(Substring(chset_pos, chset_end), charset);
>+    LossyCopyUTF16toASCII(Substring(reinterpret_cast<const char16_t *>(chset_pos),
>+                                    reinterpret_cast<const char16_t *>(chset_end)),
>+                                    charset);
[Does wwc not help here?]

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8654652 [details] [diff] [review]
fix wchar_t/char16_t mismatch in mail and mailnews

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

You should also explore fixing Neil's drive-bys.

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +1306,5 @@
>    // MAPI doesn't allow that, so we fall back on an optional
>    // name, and when all fails, on the email address.
>    aModifiedCard->GetDisplayName(properties[index_DisplayName]);
> +  const char16_t *displayName = properties[index_DisplayName].get();
> +  if (*displayName == 0) {

properties is an array of nsStrings, so I'm guessing it really should be:
if (properties[index_DisplayName].IsEmpty()) {

::: mailnews/base/util/nsMsgUtils.cpp
@@ +303,5 @@
>  
>  inline uint32_t StringHash(const nsAutoString& str)
>  {
> +    const char16_t *strbuf = str.get();
> +    return StringHash(reinterpret_cast<const char*>(strbuf),

Ugh, the context that this code is used in is so broken that I'm wondering if we shouldn't just pull the hash methods we use for hash tables instead. Unfortunately, now I'm trying to figure out if changing the value of this hash code would break anything.

Comment 11

[Stefan Sitter [:ssitter]]

Attachment: fix wchar_t/char16_t mismatch in mail and mailnews v2

Hi Neil and  Joshua, many thanks for your comments. Here is an updated patch with comments addressed. Maybe someone can push it to try when comm-central is green again to see if the other compilers are ok with the changes.

> >-  if (*properties[index_DisplayName].get() == 0) {
> This code sucks. What it actually wants to say is
> if (properties[index_DisplayName].IsEmpty()) {

I figured that much, see comment 7.

> >+        value.Value.lpszW = wwc(const_cast<char16_t *>(aValue)) ;
> Did you try wwc(aValue)?

Doesn't work. Compiler doesn't know if it should call wwc(wchar_t*) or wwc(char16_t*) otherwise.

> >-    displayName.Value.lpszW = const_cast<WCHAR *>(tempName.get()) ;
> >+    const wchar_t *tempNameValue = tempName.get();
> >+    displayName.Value.lpszW = const_cast<wchar_t *>(tempNameValue) ;
> Try (wchar_t*)tempName.get()

In my opinion we should not add C-style casts. Looks like static_cast instead of const_cast works, using that.

> >-    return StringHash(reinterpret_cast<const char*>(str.get()),
> >+    const char16_t *strbuf = str.get();
> >+    return StringHash(reinterpret_cast<const char*>(strbuf),
> >                       str.Length() * 2);
> Try (const char *)str.get()

In my opinion we should not add C-style casts. I'd rather keep the ugly C++ code here instead of hiding behind a C-style cast.

> >-    LossyCopyUTF16toASCII(Substring(chset_pos, chset_end), charset);
> >+    LossyCopyUTF16toASCII(Substring(reinterpret_cast<const char16_t *>(chset_pos),
> >+                                    reinterpret_cast<const char16_t *>(chset_end)),
> >+                                    charset);
> [Does wwc not help here?]

I can use wwc(const_cast<wchar_t *>(...) instead.

Comment 13

[Edmund Wong (:ewong)]

Comment on attachment 8658294 [details] [diff] [review]
fix wchar_t/char16_t mismatch in mail and mailnews v2

With this patch, TB builds. 

However, suite will require additional work.

Comment 14

[Stefan Sitter [:ssitter]]

SeaMoneky Bug with patch already exists: Bug 1201629.

Comment 15

[Frank-Rainer Grahl (:frg)]

The current patch for nsAbWinhelper.cpp breaks building with VS2013:

cmozmake[4]: Leaving directory 'c:/seabuild/release/comm-central/obj-x86_64-pc-mingw32/mailnews/extensions/fts3/src'
:/seamonkey/comm-central/mailnews/addrbook/src/nsAbWinHelper.cpp(548) : error C2440: 'static_cast' : cannot convert from 'const nsAS
tring_internal::char_type *' to 'WCHAR *'c:/mozilla-build/mozmake/mozmake[6]: Making `all' in `gencnval'

       Conversion loses qualifiers
c:/seabuild/release/comm-central/obj-x86_64-pc-mingw32/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsMapiAddressBook
.obj -c -I../../../dist/stl_wrappers  -DMOZ_LDAP_XPCOM -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DAB_CD=en-US
 -DNO_NSPR_10_SUPPORT -Ic:/seamonkey/comm-central/mailnews/addrbook/src -I.  -I../../../dist/include   -Ic:/seabuild/release/comm-ce
ntral/obj-x86_64-pc-mingw32/dist/include/nspr -Ic:/seabuild/release/comm-central/obj-x86_64-pc-mingw32/dist/include/cnss        -MD
           -FI ../../../dist/include/mozilla-config.h -DMOZILLA_CLIENT  -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -FS -wd4251 -wd4244
-wd4267 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -O1 -Oi -Oy   -Fdgenerated.pdb   c:/seamonkey/comm-
central/mailnews/addrbook/src/nsMapiAddressBook.cpp
:/seamonkey/comm-central/mailnews/addrbook/src/nsAbWinHelper.cpp(637) : error C2440: 'static_cast' : cannot convert from 'const nsAS
tring_internal::char_type *' to 'WCHAR *'
        Conversion loses qualifiers
nsMsgVCardService.obj
c:/seamonkey/comm-central/mailnews/addrbook/src/nsAbWinHelper.cpp(700) : error C2440: 'static_cast' : cannot convert from 'const nsA
String_internal::char_type *' to 'WCHAR *'
        Conversion loses qualifiers
nsMapiAddressBook.cppc:/seamonkey/comm-central/mozilla/config/rules.mk:958: recipe for target 'nsAbWinHelper.obj' failed

mozmake[4]: *** [nsAbWinHelper.obj] Error 2
mozmake[4]: *** Waiting for unfinished jobs....

Comment 16

[Stefan Sitter [:ssitter]]

Attachment: fix wchar_t/char16_t mismatch in mail and mailnews v3

I reverted the changes in nsAbWinHelper.cpp and went back to the solution from patch v1.

Comment 17

[Frank-Rainer Grahl (:frg)]

Compiling Seamonkey works fine with VS2013 and VS2015 using the v3 patch.

Comment 18

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/9d472671f16467be95d3b62eb4c0ff399aba8bc7
Bug 1198936 - VS2015 build fails in mailnews due to wchar_t/char16_t mismatch. r=jcranmer a=aleth