Closed
Bug 1353324
Opened 7 years ago
Closed 7 years ago
Add MakeCStringSpan variant for char16_t
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hsivonen, Assigned: hsivonen)
References
Details
Attachments
(1 file)
Bug 1353324 - Add const char16_t variant of MakeCStringSpan() and rename both to MakeStringSpan(). .
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
Unfortunately, we have zero-terminated char16_t strings in e.g. the spell checker. I need to make spans of those in bug 1261841.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8854387 [details] Bug 1353324 - Add const char16_t variant of MakeCStringSpan() and rename both to MakeStringSpan(). . https://reviewboard.mozilla.org/r/126314/#review129042 r- due to the nsCharTraits.h issue and corresponding fallout on try. ::: mfbt/Span.h:36 (Diff revision 1) > #include <algorithm> > #include <array> > #include <cstring> > #include <iterator> > > +#include "nsCharTraits.h" This isn't going to work very well for things that aren't linked into libxul (e.g. standalone JS builds, some tests). The obvious thing to do here is to try and lift the code from this header into MFBT where everybody can use it, but there's a lot of cruft in that header that I don't know that we want to import into MFBT. We could just inline the `char16_t` strlen here? ::: mfbt/Span.h:1028 (Diff revision 1) > +inline Span<const char16_t> > +MakeCStringSpan(const char16_t* aStr) The naming here is contra Gecko string conventions. I guess you can argue that `CString` here is more like "C-style, null-terminated string" than the Gecko 8-bit character string. I'm inclined to let this stay, since its use should be uncommon...
Attachment #8854387 -
Flags: review?(nfroyd) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8854387 [details] Bug 1353324 - Add const char16_t variant of MakeCStringSpan() and rename both to MakeStringSpan(). . https://reviewboard.mozilla.org/r/126314/#review129042 > This isn't going to work very well for things that aren't linked into libxul (e.g. standalone JS builds, some tests). The obvious thing to do here is to try and lift the code from this header into MFBT where everybody can use it, but there's a lot of cruft in that header that I don't know that we want to import into MFBT. We could just inline the `char16_t` strlen here? I added an inline implementation of `char16_t` `strlen()` in the `span_details` namespace. > The naming here is contra Gecko string conventions. I guess you can argue that `CString` here is more like "C-style, null-terminated string" than the Gecko 8-bit character string. > > I'm inclined to let this stay, since its use should be uncommon... Renamed both to `MakeStringSpan`.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8854387 [details] Bug 1353324 - Add const char16_t variant of MakeCStringSpan() and rename both to MakeStringSpan(). . https://reviewboard.mozilla.org/r/126314/#review129434 Thanks.
Attachment #8854387 -
Flags: review?(nfroyd) → review+
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4bcbc3631edf Add const char16_t variant of MakeCStringSpan() and rename both to MakeStringSpan(). r=froydnj.
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4bcbc3631edf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•