Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Description • 7 years ago

Unfortunately, we have zero-terminated char16_t strings in e.g. the spell checker. I need to make spans of those in bug 1261841.

	Nathan Froyd [:froydnj]
	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...

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) 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`.

	Nathan Froyd [:froydnj]
	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.

	Pulsebot
	Comment 6 • 7 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 7 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4bcbc3631edf