Consider removing |wwc| functions

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

Attachments

(3 attachments)

nsString.h defines some sort of odd cast helper function |wwc| [1] for wchar_t. I believe |char16ptr_t| [2] covers what we need here.

[1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/string/nsString.h#152-192
[2] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/mfbt/Char16.h
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14462ed43b467d47168205a90277243d111fbba6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce0ab1053160c32fa90eb8dc484df4ff401fd574
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd015b89c49e689fae3ff06019c252acfbc0de0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8afa23e4b28c64a138455e23068f5e4e34216594
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9549bc10f35f5b8109a31b1cbc6132134728437
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c61ba32e594aca1ca52d341cf3df09791d0da50a
Created attachment 8855131 [details] [diff] [review]
Part 1: Allow implicit conversion of non-const char16ptr_t to wchar*

This implements an implicit conversion from a non-const char16ptr_t to wchar*.
This is needed when passing a mutable string buffer to a windows function, ie:

> nsString str;
> str.SetLength(100);
> WindowsFunctionW(str.get());
Attachment #8855131 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Created attachment 8855133 [details] [diff] [review]
Remove wwc functions

This removes the use of |wwc| functions in favor of char16ptr_t's implicit
conversion operators.
Attachment #8855133 - Flags: review?(nfroyd)
The latest try run was happy (aside from the S builds, but I'm not taking the blame for that):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b89ab3192685d29ac40a4e1fc951afdddeb14c63
Jacek, can you see if this patch set builds for you?  I tried looking at the history of bug 928351, which added this wrapper to Char16.h, but I can't find any explicit rationale for why we didn't include non-const operator wchar_t*() functions.  Thanks!
Flags: needinfo?(jacek)

Comment 11

7 months ago
I can't testing ATM (mingw-w64 build is broken by other bugs), but I don't see any reason it wouldn't work there.

The rationale to not add this was to be as close to |const wchar_t*| as possible (back then MSVC builds didn't use char16ptr_t) and you can't implicitly cast const pointer to non-const pointer. By that logic, your change makes the code less type-safe by dropping requirement to do const cast. Still, that makes the code a bit cleaner, so that's probably ok.
Flags: needinfo?(jacek)
Attachment #8855131 - Flags: review?(nfroyd) → review+
Attachment #8855133 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc441fc5860b4c6ebc22c3d261aa9025dfe15da
Bug 1353593 - Part 1: Allow implicit conversion of non-const char16ptr_t to wchar*. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/c16b53fc17d9155299c00e49c8287e24c1aeb1bf
Bug 1353593 - Part 2: Remove wwc functions. r=froydnj
Backed out for windows build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105969634&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b11f8753daed51e71663be9976ef846498dfed7
Flags: needinfo?(erahm)
(In reply to Wes Kocher (:KWierso) from comment #13)
> Backed out for windows build failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=105969634&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 8b11f8753daed51e71663be9976ef846498dfed7

Looks like a new usage of |wwc| was added in the last two months.
Flags: needinfo?(erahm)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bf91aa2b0635f71afc97ae3869a3b2faf9bc0d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/56bbb6d5d58728d56b8ee85449b2fbc7a69604ad
Bug 1353593 - Part 1: Allow implicit conversion of non-const char16ptr_t to wchar*. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/11c7b498a83650e1d9f658700ff06d07a9106329
Bug 1353593 - Part 2: Remove wwc functions. r=froydnj
Backed out for Windows 2012 static build bustage in Unified_cpp_gfx_thebes0.obj:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3955d49d5cd776dfb0849ce44fd7975e49fc34f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e273a44edbae36c831756124c70e35e6e98d6554

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=11c7b498a83650e1d9f658700ff06d07a9106329&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105998366&repo=mozilla-inbound

00:27:46     INFO -  In file included from z:/task_1497051273/build/src/obj-firefox/gfx/thebes/Unified_cpp_gfx_thebes0.cpp:11:
00:27:46     INFO -  z:/task_1497051273/build/src/gfx/thebes/D3D11Checks.cpp(162,99):  error: use of overloaded operator '<<' is ambiguous (with operand types 'mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>' and 'char16ptr_t')
00:27:46     INFO -            gfxCriticalError(CriticalLog::DefaultOptions(false)) << "DisplayLink: too old version " << displayLinkModuleVersionString.get();
00:27:46     INFO -            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(310,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(int aInt) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(286,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(char aChar) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(304,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(bool aBool) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(316,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(unsigned int aInt) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(322,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(long aLong) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(328,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(unsigned long aLong) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(334,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(long long aLong) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(340,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(unsigned long long aLong) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(346,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(Float aFloat) {
00:27:46     INFO -         ^
00:27:46     INFO -  z:/task_1497051273/build/src/obj-firefox/dist/include/mozilla/gfx/Logging.h(352,8):  note: candidate function
00:27:46     INFO -    Log &operator <<(double aDouble) {
00:27:46     INFO -         ^
00:27:46     INFO -  1 error generated.
00:27:46     INFO -  z:/task_1497051273/build/src/config/rules.mk:1064: recipe for target 'Unified_cpp_gfx_thebes0.obj' failed
00:27:46     INFO -  mozmake.EXE[5]: *** [Unified_cpp_gfx_thebes0.obj] Error 1
Flags: needinfo?(erahm)
Looks like clang-cl is having issues, we can probably do an explicit cast and move on.
Flags: needinfo?(erahm)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bf77b1f5a8ca37661c5ee0d46a4253c4d7be1e
Created attachment 8876895 [details] [diff] [review]
Part 1.1: Fix usage of char16ptr_t in a graphics logging message

This message intends to print out a string but was inadvertantly converting
the string pointer to a hex value. Adding an implicit conversion of char16ptr_t
to |wchar*| breaks this, so we just update the code to actually convert and
print the string instead.

MozReview-Commit-ID: 90luEnoysX3
Attachment #8876895 - Flags: review?(milan)
Comment on attachment 8876895 [details] [diff] [review]
Part 1.1: Fix usage of char16ptr_t in a graphics logging message

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

::: gfx/thebes/D3D11Checks.cpp
@@ +159,5 @@
>            return false;
>          }
>          if (displayLinkModuleVersion <= V(8,6,1,36484)) {
> +          NS_ConvertUTF16toUTF8 version(displayLinkModuleVersionString);
> +          gfxCriticalError(CriticalLog::DefaultOptions(false)) << "DisplayLink: too old version " << version.get();

I probably would have written
gfxCriticalError ... << NS_ConvertUTF16toUTF8(displayLinkModuleVersionString).get();
but this works too.
Attachment #8876895 - Flags: review?(milan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f2a58bedb3f533453c3e2823b60671fe9d5de1
Bug 1353593 - Part 1: Allow implicit conversion of non-const char16ptr_t to wchar*. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5ccd1d096eb818d593c7fa9b9332e51a3dd262
Bug 1353593 - Part 1.1: Fix usage of char16ptr_t in a graphics logging message. r=milan

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f6111bc6d0abfa5199a7e8d3549bfed8ff29ba
Bug 1353593 - Part 2: Remove wwc functions. r=froydnj

Comment 23

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6f2a58bedb3
https://hg.mozilla.org/mozilla-central/rev/ca5ccd1d096e
https://hg.mozilla.org/mozilla-central/rev/a9f6111bc6d0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.