Closed Bug 1353593 Opened 7 years ago Closed 7 years ago

Consider removing |wwc| functions

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

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
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
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)
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+
(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)
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)
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
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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...
Depends on: 1426313
(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.
(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)
(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.
(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)
(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.