Closed
Bug 944905
Opened 10 years ago
Closed 10 years ago
Fix char16_t/wchar_t mismatch in xpcom/
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: jacek, Assigned: jacek)
References
Details
Attachments
(2 files)
31.52 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
6.97 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #8340648 -
Flags: review?(benjamin)
Comment 1•10 years ago
|
||
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
Attachment #8340648 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 2•10 years ago
|
||
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.
Whiteboard: leave open
Comment 3•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28f646255d25
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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,
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8356133 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8356133 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f381426af1
Whiteboard: leave open
Comment 8•10 years ago
|
||
backedout because of bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=32636076&tree=Mozilla-Inbound on windows builds
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c669ccc6e055
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•