1228482 - nsEdgeReadingListExtractor.cpp(205): error C2665 with VS2015

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

0:49.92 central/browser/components/migration/nsEdgeReadingListExtractor.cpp(205): error C2665: 'swprintf': none of the 2 overloads could convert all the argument types
 0:49.92 C:\Program Files (x86)\Windows Kits\10\include\10.0.10240.0\ucrt\corecrt_wstdio.h(1820): note: could be 'int swprintf(wchar_t *const ,const wchar_t *const ,...) throw()'
 0:49.92 C:\Program Files (x86)\Windows Kits\10\include\10.0.10240.0\ucrt\corecrt_wstdio.h(1463): note: or       'int swprintf(wchar_t *const ,const size_t,const wchar_t *const ,...)'
 0:49.92 central/browser/components/migration/nsEdgeReadingListExtractor.cpp(205): note: while trying to match the argument list '(wchar_t *, int, const char16_t [44], const JET_ERR)'
 0:49.92 central/browser/components/migration/nsEdgeReadingListExtractor.cpp(206): error C2664: 'nsresult nsIConsoleService::LogStringMessage(const char16_t *)': cannot convert argument 1 from 'wchar_t *' to 'const char16_t *'
 0:49.92 central/browser/components/migration/nsEdgeReadingListExtractor.cpp(206): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1228482 - Fix compile error with VS2015 and memory leak in nsEdgeReadingListExtractor.

Bug 1228482 - Fix compile error with VS2015 and memory leak in nsEdgeReadingListExtractor.

Comment 2

[:Gijs (he/him)]

Ugh. This is the third time I've done this now. When I wrote the patch in bug 1225798, I explicitly went looking for what the previous adjustments had done to make sure that what I would be doing would not cause the same kind of breakage again (bug 1213246, bug 1200187). I was under the impression that MOZ_UTF16 was supposed to get around this issue (that was certainly how some of the other breakage was fixed). I couldn't find any information on MDN, and did find out that we do our own wrapping of some of the wide string types. For someone not familiar with this and not in possession of msvc2015 this seems impossible to get right.

Can you (or someone else who knows) please write a document on MDN with details about how to deal with this kind of problem correctly?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to :Gijs Kruitbosch from comment #2)
> I was under the impression
> that MOZ_UTF16 was supposed to get around this issue (that was certainly how
> some of the other breakage was fixed).

MOZ_UTF16 is not. MOZ_UTF16("string") is simply extended to L"string" (on VS2013) and u"string" (otherwise). The former has type "const wchar_t[]" and the latter is "const char16_t[]". On VS2013, there is no native char16_t, so we alias char16_t to wchar_t, however on VS2015, those two become two independent types.

> I couldn't find any information on
> MDN, and did find out that we do our own wrapping of some of the wide string
> types. For someone not familiar with this and not in possession of msvc2015
> this seems impossible to get right.

I totally agree. I could get it wrong as well if I do not build on VS2015.

> Can you (or someone else who knows) please write a document on MDN with
> details about how to deal with this kind of problem correctly?

I guess I can... Leave a ni? for myself.

Comment 4

[Mike Hommey [:glandium] (high latency)]

You should just avoid swprintf and wchar_t altogether. It's not like we have an entire String API in xpcom/ to do string formatting.

Comment 5

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #4)
> You should just avoid swprintf and wchar_t altogether. It's not like we have
> an entire String API in xpcom/ to do string formatting.

... and yet the console service's API takes a wstring, and has no API that takes an XPCOM string. It has one that takes an nsIConsoleMessage, but that's harder to construct and overkill for what is going on here.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8692785 [details]
MozReview Request: Bug 1228482 - Fix compile error with VS2015 and memory leak in nsEdgeReadingListExtractor.

https://reviewboard.mozilla.org/r/26323/#review23779

Looks good to me.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f04c4b2cf60

Comment 8

[Mike Hommey [:glandium] (high latency)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > You should just avoid swprintf and wchar_t altogether. It's not like we have
> > an entire String API in xpcom/ to do string formatting.
> 
> ... and yet the console service's API takes a wstring, and has no API that
> takes an XPCOM string. It has one that takes an nsIConsoleMessage, but
> that's harder to construct and overkill for what is going on here.

a wstring is a char16_t.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7675a900e87

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Well, I guess instead of adding some document to describe this, it is probably better to just break compiling on VS2013 if someone tries to do implicit conversion between wchar_t and char16_t. That says, we should not typedef wchar_t to char16_t on VS2013, but instead, declaring an independent class to simulate all its behaviors except the implicit conversion from/to wchar_t.

Comment 11

[Mike Hommey [:glandium] (high latency)]

The best thing to do is to avoid wchar_t unless you're _very_ sure there's no other way than to use it. IOW, unless Windows API that require it are involved, you shouldn't be using it. And you should avoid system string functions that require it (like swprintf) and use our own String APIs instead. That's the rule of thumb.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f04c4b2cf60
https://hg.mozilla.org/mozilla-central/rev/b7675a900e87