Remove redundant #ifdef MOZ_USE_CHAR16_WRAPPER wrapped overload

RESOLVED FIXED in Firefox 44

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

There are various places have pattern like this:
> void AppendUTF16toUTF8(const char16_t* aSource, nsACString& aDest);
> #ifdef MOZ_USE_CHAR16_WRAPPER
> inline void AppendUTF16toUTF8(char16ptr_t aSource, nsACString& aDest)
> {
>   return AppendUTF16toUTF8(static_cast<const char16_t*>(aSource), aDest);
> }
> #endif
However, it is totally unnecessary because char16ptr_t is designed to always be implicitly-convertible to and from char16_t*.
I think this is needed to convert from wchar_t* on MinGW or MSVC2015+, no?
Flags: needinfo?(jacek)
Yes. We can just use like:
> void AppendUTF16toUTF8(const char16ptr_t aSource, nsACString& aDest);
instead of two overloads.
Summary: Remove redundant #ifdef MOZ_USE_CHAR16_WRAPPER wrapped override → Remove redundant #ifdef MOZ_USE_CHAR16_WRAPPER wrapped overload
Created attachment 8671743 [details] [diff] [review]
patch
Assignee: nobody → quanxunzhen
Attachment #8671743 - Flags: review?(nfroyd)

Comment 4

2 years ago
As far as I remember those are overloaded to avoid cases where we'd need two implicit casts to call the function (like <a class with const char16_t* cast operator> -> const char16_t -> char16ptr_t). Such cases would cause compile error without overloading. I just tried a build with your patch on 64-bit mingw and it works fine. Thus, I'm fine with the change.
Flags: needinfo?(jacek)
(In reply to Jacek Caban from comment #4)
> As far as I remember those are overloaded to avoid cases where we'd need two
> implicit casts to call the function (like <a class with const char16_t* cast
> operator> -> const char16_t -> char16ptr_t). Such cases would cause compile
> error without overloading. I just tried a build with your patch on 64-bit
> mingw and it works fine. Thus, I'm fine with the change.

I wish the preprocessor macro in Char16.h for determining MOZ_USE_CHAR16_WRAPPER was a little more transparent (maybe __MINGW32__ || __MINGW64__ ?).  It's not immediately clear to me why these particular overloads can be deleted, but other uses of MOZ_USE_CHAR16_WRAPPER are OK...but if things build as-is, I guess we can proceed.
Comment on attachment 8671743 [details] [diff] [review]
patch

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

rs=me based on Jacek's testing.
Attachment #8671743 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #5)
> I wish the preprocessor macro in Char16.h for determining
> MOZ_USE_CHAR16_WRAPPER was a little more transparent (maybe __MINGW32__ ||
> __MINGW64__ ?).

Although it was introduced for MinGW, MSVC2015 and clang-cl also use this macro now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f5d00fc36b39e06d663f18218a4af732d94683
Bug 1213130 - Make several string function to accept char16ptr_t instead of char16_t*, and remove redundant overloads. rs=froydnj
(In reply to Nathan Froyd [:froydnj] from comment #5)
> I wish the preprocessor macro in Char16.h for determining
> MOZ_USE_CHAR16_WRAPPER was a little more transparent (maybe __MINGW32__ ||
> __MINGW64__ ?).  It's not immediately clear to me why these particular
> overloads can be deleted, but other uses of MOZ_USE_CHAR16_WRAPPER are
> OK...but if things build as-is, I guess we can proceed.

Other uses are mostly inside generic classes, where they use char_type* not char16_t*. We may be able to remove them, but that's non-trivial, at least not as trivial as this change.
https://hg.mozilla.org/mozilla-central/rev/57f5d00fc36b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.