Closed
Bug 1353593
Opened 7 years ago
Closed 7 years ago
Consider removing |wwc| functions
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: erahm, Assigned: erahm)
References
(Depends on 1 open bug)
Details
Attachments
(3 files)
1.04 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
9.22 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14462ed43b467d47168205a90277243d111fbba6
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce0ab1053160c32fa90eb8dc484df4ff401fd574
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd015b89c49e689fae3ff06019c252acfbc0de0
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8afa23e4b28c64a138455e23068f5e4e34216594
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9549bc10f35f5b8109a31b1cbc6132134728437
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c61ba32e594aca1ca52d341cf3df09791d0da50a
Assignee | ||
Comment 7•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
This removes the use of |wwc| functions in favor of char16ptr_t's implicit conversion operators.
Attachment #8855133 -
Flags: review?(nfroyd)
Assignee | ||
Comment 9•7 years ago
|
||
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
Comment 10•7 years ago
|
||
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 years 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)
Updated•7 years ago
|
Attachment #8855131 -
Flags: review?(nfroyd) → review+
Updated•7 years ago
|
Attachment #8855133 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bf91aa2b0635f71afc97ae3869a3b2faf9bc0d9
Assignee | ||
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
Looks like clang-cl is having issues, we can probably do an explicit cast and move on.
Flags: needinfo?(erahm)
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bf77b1f5a8ca37661c5ee0d46a4253c4d7be1e
Assignee | ||
Comment 20•7 years ago
|
||
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+
Assignee | ||
Comment 22•7 years ago
|
||
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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Comment 25•6 years ago
|
||
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #7) > 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()); We really should not allow implicit violation of constness. I'm surprised this slipped though a review...
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #25) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #7) > > 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()); > > We really should not allow implicit violation of constness. I'm surprised > this slipped though a review... The intention was to allow converting a non-const char16_t to a non-const wchar, if that's not the case we should fix it.
Comment 27•6 years ago
|
||
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #26) > The intention was to allow converting a non-const char16_t to a non-const > wchar, if that's not the case we should fix it. How does it prevent const char16_t* p1; char16ptr_t p2 = p1; wchar_t* p3 = p2; ?
Flags: needinfo?(erahm)
Comment 28•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #27) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #26) > > The intention was to allow converting a non-const char16_t to a non-const > > wchar, if that's not the case we should fix it. > > How does it prevent > const char16_t* p1; > char16ptr_t p2 = p1; > wchar_t* p3 = p2; > ? Answer: it does not. .get() should not be used to get a writable buffer. We should have changed .BeginWriting() to make the implicit conversion possible.
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #28) > (In reply to Masatoshi Kimura [:emk] from comment #27) > > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > > #26) > > > The intention was to allow converting a non-const char16_t to a non-const > > > wchar, if that's not the case we should fix it. > > > > How does it prevent > > const char16_t* p1; > > char16ptr_t p2 = p1; > > wchar_t* p3 = p2; > > ? I'm okay with a non-const char16ptr_t converting to a non-const whcar_t*, but I can see how this might be problematic. > Answer: it does not. > .get() should not be used to get a writable buffer. That seems reasonable, although not particularly enforced. > We should have changed .BeginWriting() to make the implicit conversion possible. Can you file a bug for this proposed change? It's not clear how we would do this. My thought is we should have a concept of |const char16ptr_t| vs |char16ptr_t|, then we could have: > const char16ptr_t BeginReading() const; // only const ptrs can be retrieved > char16ptr_t BeginWriting(); // mutable pointers can be retrieved But maybe we'd have |mutable_char16ptr_t BeginWriting()| or something to that effect. Either way lets discuss in a separate bug.
Flags: needinfo?(erahm)
Comment 30•6 years ago
|
||
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #29) > Can you file a bug for this proposed change? Repurposed bug 1426313.
You need to log in
before you can comment on or make changes to this bug.
Description
•