1442648 - Build error during Windows cross-compile of Thunderbird on Linux (type mismatch in nsMsgImapSearch.cpp)

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Sukhbir Singh]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180219150722

Steps to reproduce:

On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows on Linux using mingw-w64, the . Please see the attached log for details but a brief summary is:

{{{
20:38.34 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp: In static member function 'static nsresult nsMsgSearchOnlineMail::Encode(nsCString&, nsISupportsArray*, const char16_t*)':
20:38.34 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:122:43: error: call of overloaded 'NS_IsAscii(char16ptr_t)' is ambiguous
20:38.34          asciiOnly = NS_IsAscii(pchar.get());
20:38.34                                            ^
20:38.34 In file included from /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/msgCore.h:15:0,
20:38.35                  from /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:5:
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/nsCRTGlue.h:101:6: note: candidate: bool NS_IsAscii(char16_t)
20:38.35  bool NS_IsAscii(char16_t aChar);
20:38.35       ^
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/obj-mingw/dist/include/nsCRTGlue.h:102:6: note: candidate: bool NS_IsAscii(const char16_t*)
20:38.35  bool NS_IsAscii(const char16_t* aString);
20:38.35       ^
20:38.35 /var/tmp/build/thunderbird-48f960ae7db5/mailnews/base/search/src/nsMsgImapSearch.cpp:131:48: error: operands to ?: have different types 'char16ptr_t' and 'const char16_t*'
20:38.35    char *csname = GetImapCharsetParam(asciiOnly ? usAsciiCharSet.get() : destCharset);
}}}

I have attached a patch (based on changes from #928351) which fixes this issue.

Comment 1

[Sukhbir Singh]

Attachment: 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Comment 2

[Sukhbir Singh]

Attachment: typemistach

error log

Comment 3

[Sukhbir Singh]

(In reply to Sukhbir Singh from comment #0)
> User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101
> Firefox/58.0
> Build ID: 20180219150722
> 
> Steps to reproduce:
> 
> On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows
> on Linux using mingw-w64, the . Please see the attached log for details but
> a brief summary is:

Seems like I missed a sentence here. 

> On cross-compiling comm-esr52 (THUNDERBIRD_52_6_0_RELEASE tag) for Windows on Linux using mingw-w64, the build fails due to a type mismatch  in nsMsgImapSearch.cpp.

Comment 4

[Tom Prince [:tomprince]]

Comment on attachment 8955525 [details] [diff] [review]
0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Jorg, I'm not sure who the best person to review this is. Can you redirect as appropriate?

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8955525 [details] [diff] [review]
0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

I can take this review (being a mailnews peer).

Usually contributors provide patches that apply to trunk. Looking at the patch, there are many trailing spaces which I have removed from the code base in the last few months, so I'm pretty sure this patch won't apply to trunk.

The first hunk is OK, for the second hunk I have this comment:
Line 121:   nsAutoString usAsciiCharSet(NS_LITERAL_STRING("us-ascii"));
Line 123:   char *csname = GetImapCharsetParam(asciiOnly ? usAsciiCharSet.get() : destCharset);
Line 128:     asciiOnly ?  usAsciiCharSet.get(): destCharset,
Line 129:     asciiOnly ?  usAsciiCharSet.get(): destCharset, false);

Looking at the declaration and use of usAsciiCharSet, it is an nsAutoString for no good reason at all. So can we do something like:
char16_t* usAsciiCharSet = u"us-ascii" and be done with this (of course removing all the .get()s)?

Comment 6

[Sukhbir Singh]

(In reply to Jorg K (GMT+1) from comment #5)
> Comment on attachment 8955525 [details] [diff] [review]
> 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch
> 
> I can take this review (being a mailnews peer).

Thanks for the review, Jorg.

> Usually contributors provide patches that apply to trunk. Looking at the
> patch, there are many trailing spaces which I have removed from the code
> base in the last few months, so I'm pretty sure this patch won't apply to
> trunk.

Ah I see. I thought I should do it with the tag I am working; I have fixed it so that it applies to trunk.

> The first hunk is OK, for the second hunk I have this comment:
> Line 121:   nsAutoString usAsciiCharSet(NS_LITERAL_STRING("us-ascii"));
> Line 123:   char *csname = GetImapCharsetParam(asciiOnly ?
> usAsciiCharSet.get() : destCharset);
> Line 128:     asciiOnly ?  usAsciiCharSet.get(): destCharset,
> Line 129:     asciiOnly ?  usAsciiCharSet.get(): destCharset, false);
> 
> Looking at the declaration and use of usAsciiCharSet, it is an nsAutoString
> for no good reason at all. So can we do something like:
> char16_t* usAsciiCharSet = u"us-ascii" and be done with this (of course
> removing all the .get()s)?

You are correct, it's better if this is a const char16_t* instead. I have updated the patch.

Comment 7

[Sukhbir Singh]

Attachment: 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Hmm, as I said, trunk does not have the trailing spaces any more that are in your patch. I've rebased it, this patch now applies.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 0001-Bug-1442648-Fix-mismatched-types-in-mingw-w64-builds.patch

Fixed more white-space issues.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d0eae7f808b9
Fix mismatched types in mingw-w64 builds (const char16_t*/char16ptr_t). r=jorgk

Comment 11

[Sukhbir Singh]

Thanks for the merge Jorg. (Sorry about the whitespace issues, it was my editor on a remote machine missing the .vimrc :)

Comment 12

[:aceman]

Interesting the patch could drop some .get()s. Was the code wrong in the first place, but worked on other compilers due to some automatic casting?

Comment 13

[Jorg K (CEST = GMT+2)]

Well, there is some magic going on with char16_t*/char16ptr_t:
https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/mfbt/Char16.h#35

There are other static_cast<const char16_t*> casts in the code base, for example here, so I think we did the right thing:
https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/xpcom/string/nsTSubstring.h#219

I'm just surprised that this should be the only compiler error in mingw-w64 builds.

Comment 14

[Sukhbir Singh]

(In reply to Jorg K (GMT+1) from comment #13)
> Well, there is some magic going on with char16_t*/char16ptr_t:
> https://searchfox.org/mozilla-central/rev/
> 908e3662388a96673a0fc00b84c47f832a5bea7c/mfbt/Char16.h#35

Yes, this is from bug 928351.

> There are other static_cast<const char16_t*> casts in the code base, for
> example here, so I think we did the right thing:
> https://searchfox.org/mozilla-central/rev/
> 908e3662388a96673a0fc00b84c47f832a5bea7c/xpcom/string/nsTSubstring.h#219
> 
> I'm just surprised that this should be the only compiler error in mingw-w64
> builds.

This is the only type mismatch error that I came across as of now but the build is still not complete due to missing headers in mingw-w64. I opened bug 1442034 but since cross-compiling Thunderbird is not supported that was closed. So now I will try adding those headers (for example, SearchAPI.h as in bug 1442034) to mingw-w64 instead.