Closed Bug 1393235 Opened 7 years ago Closed 7 years ago

Fix improper usages of string functions

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #8900477 - Flags: review?(n.nethercote)
(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.
(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 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+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: