944905 - Fix char16_t/wchar_t mismatch in xpcom/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jacek Caban]

Attachment: patch.diff

This is followup of bug 928351. This uses wchar_t for Windows API calls and required casts for places mixing those with char16_t-based functions.

Comment 1

[Benjamin Smedberg]

Comment on attachment 8340648 [details] [diff] [review]
patch.diff

Why is the cast in nsTextFormatter.cpp necessary?

I almost feel in some of these cases we'd be better off with an overloaded helper function:

wchar_t* wcc(char16_t*)  (short for wchar_convert)
char16_t* wcc(wchar_t*)
and const versions

The reinterpret_casts here are ugly and make us less typesafe for future code refactoring.

r=me on everything *except* the reinterpret_casts and nsTextFormatter.cpp

Comment 2

[Jacek Caban]

Thanks for the review. I landed the r+ed parts of the patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f646255d25

I will work on reinterpret_casts soon. nsTextFormated.cpp had some problems with exact types required for templates AFAIR. I will recheck.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28f646255d25

Comment 4

[Jacek Caban]

Sorry that it took a while.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Comment on attachment 8340648 [details] [diff] [review]
> patch.diff
> 
> Why is the cast in nsTextFormatter.cpp necessary?


That's because the returned type of get() is char16ptr_t, which is different type than const char16_t* (on mingw), so the conditional expression has different types in its branches. 

> I almost feel in some of these cases we'd be better off with an overloaded
> helper function:
> 
> wchar_t* wcc(char16_t*)  (short for wchar_convert)
> char16_t* wcc(wchar_t*)
> and const versions
> 
> The reinterpret_casts here are ugly and make us less typesafe for future
> code refactoring.
> 
> r=me on everything *except* the reinterpret_casts and nsTextFormatter.cpp

Comment 5

[Jacek Caban]

I accidentally posted before finishing post.

(In reply to Jacek Caban from comment #4)
> Sorry that it took a while.
> 
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> > Comment on attachment 8340648 [details] [diff] [review]
> > patch.diff
> > 
> > Why is the cast in nsTextFormatter.cpp necessary?
> 
> 
> That's because the returned type of get() is char16ptr_t, which is different
> type than const char16_t* (on mingw), so the conditional expression has
> different types in its branches. 

Looking back at this, it seems like we could simply use MOZ_UTF16 instead. I don't see why the literal string was introduced there.

> > I almost feel in some of these cases we'd be better off with an overloaded
> > helper function:
> > 
> > wchar_t* wcc(char16_t*)  (short for wchar_convert)
> > char16_t* wcc(wchar_t*)

OK. I wasn't sure where this should go to, so I added it to nsString.h. This meant that I couldn't use it in nsVersionComparator.h, which may be used without nsString.h.

> > and const versions

const variants are handled by char16ptr_t. We may explicitly cast to char16ptr_t if needed, which will later use implicit cast to the right type.

Also,

Comment 6

[Jacek Caban]

Attachment: remaining part

Comment 7

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f381426af1

Comment 8

[Carsten Book [:Tomcat]]

backedout because of bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=32636076&tree=Mozilla-Inbound on windows builds

Comment 9

[[e-mail]]

Comment on attachment 8340648 [details] [diff] [review]
patch.diff

> #ifdef XP_WIN
> struct NS_COM_GLUE VersionW
> {
>   VersionW(const PRUnichar *versionStringW)
I couldn't find any consumers of this, so I can't tell whether it should have been made wchar_t instead.

>     // Cast away const-ness here because WinAPI functions don't understand it, 
>     // the path is used for [in] parameters only however so it's safe. 
>-    WCHAR *path = const_cast<WCHAR*>(mFollowSymlinks ? mResolvedPath.get() 
>-                                                        : mWorkingPath.get());
>+    const WCHAR *path = mFollowSymlinks ? mResolvedPath.get() : mWorkingPath.get();
So you're no longer casting away const-ness here? Comment is out of date.

> nsLocalFile::SetModDate(PRTime aLastModifiedTime, const PRUnichar *filePath)
> {
>     // The FILE_FLAG_BACKUP_SEMANTICS is required in order to change the
>     // modification time for directories.
>-    HANDLE file = ::CreateFileW(filePath,          // pointer to name of the file
>+    HANDLE file = ::CreateFileW(char16ptr_t(filePath), // pointer to name of the file
Would it have been better to change filePath to const wchar_t *? (Both callers pass a .get() to it.)

> // moved from widget/windows/nsToolkit.cpp
> int32_t 
> NS_ConvertAtoW(const char *aStrInA, int aBufferSize, PRUnichar *aStrOutW)
...
> int32_t 
> NS_ConvertWtoA(const PRUnichar *aStrInW, int aBufferSizeOut,
>                char *aStrOutA, const char *aDefault)
[I couldn't find a caller for these either...]

Comment 10

[[e-mail]]

Comment on attachment 8356133 [details] [diff] [review]
remaining part

>-    NS_NAMED_LITERAL_STRING(nullstr, "(null)");
>-
>-    return fill2(ss, s ? s : nullstr.get(), slen, width, flags);
>+    return fill2(ss, s ? s : MOZ_UTF16("(null)"), slen, width, flags);
FYI MOZ_UTF16 is pretty new, which explains why the old code used a named literal string.

Comment 11

[Jacek Caban]

Neil, thanks for comments.

I relanded a fixed version with changes for Neil's comments and build fixes:

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c669ccc6e055