Closed
Bug 1393235
Opened 7 years ago
Closed 7 years ago
Fix improper usages of string functions
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
This fixes usages of `Find`, `RFind` and the equality operator that kind of
work right now but will break with the proper type checking of a templatized
version of the string classes.
For `Find` and `RFind` it appears that `nsCString::(R)Find("foo", 0)` calls
were being coerced to the `Find(char*, bool, int, int)` versions. The intent was
probably to just start searching from position zero.
For the equality operator, the type of nullptr is nullptr_t rather than
char(16_t)* so we'd need to add an operator overload that takes nullptr_t. In
this case just using `IsVoid` is probably more appropriate.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8900477 -
Flags: review?(n.nethercote)
Comment 2•7 years ago
|
||
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #0)
> For the equality operator, the type of nullptr is nullptr_t rather than
> char(16_t)* so we'd need to add an operator overload that takes nullptr_t. In
> this case just using `IsVoid` is probably more appropriate.
I'm surprised those are equivalent.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #2)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #0)
> > For the equality operator, the type of nullptr is nullptr_t rather than
> > char(16_t)* so we'd need to add an operator overload that takes nullptr_t. In
> > this case just using `IsVoid` is probably more appropriate.
>
> I'm surprised those are equivalent.
Well I think the intent of |nsCString != nullptr| is the same as |!IsVoid()|, but you're right in practice [1] it's more of `!IsEmpty()`. Not sure if we should change it to do the "right" thing, or keep existing behavior.
[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/xpcom/string/nsTSubstring.cpp#794-807
Comment 4•7 years ago
|
||
Comment on attachment 8900477 [details] [diff] [review]
Fix improper usages of string functions
Review of attachment 8900477 [details] [diff] [review]:
-----------------------------------------------------------------
Those Find() functions are awful.
::: dom/media/gmp/ChromiumCDMChild.cpp
@@ +231,5 @@
> // As laid out in the Chromium CDM API, if the CDM fails to load
> // a session it calls OnResolveNewSessionPromise with nullptr as the sessionId.
> // We can safely assume this means that we have failed to load a session
> // as the other methods specify calling 'OnRejectPromise' when they fail.
> + bool loadSuccessful = !aSessionId.IsVoid();
The incoming strings can be `nsCString(nullptr, 0)`, which I think becomes a non-void empty string, so this should probably be `!aSessionId.IsEmpty()` or equivalent.
Attachment #8900477 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 5•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5556637f69d48d84e51bfdb4c205a41ef2700e2
Bug 1393235 - Fix improper usages of string functions. r=njn
Comment 6•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•