1448591 - Make MakeStringSpan(nullptr) return an empty span

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Being able to pass nullptr to MakeStringSpan() and getting back an empty span turns out to be important for the ergonomics of migrating legacy C string code to spans.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

Review commit: https://reviewboard.mozilla.org/r/230948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230948/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

https://reviewboard.mozilla.org/r/230948/#review236624

I'm not sure I fully grok the "...important for the ergonomics of migrating legacy C string code to spans."  Is this shorthand for converting code at some boundary between C-style APIs and Span-style APIs?  If it's just dealing with code at the boundary, I'm unsure that saving an explicit check for nullptr is worth it.  And I'm not sure that handling the case of "might not have provided a string" to be the same as "zero-length string provided" is really correct.

Or the case being handled something else?

::: mfbt/Span.h:1069
(Diff revision 1)
> +  if (!aZeroTerminated) {
> +    return Span<const char>();
> +  }

Two things:

1. This behavior should be documented.  (The behavior is convenient, but it's not the universal behavior for zero-terminated string functions; e.g. `strlen` does not handle null input well.)
2. This behavior needs explicit tests.

::: mfbt/Span.h:1081
(Diff revision 1)
> +  if (!aZeroTerminated) {
> +    return Span<const char16_t>();
> +  }

Same comments apply here as well.

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

https://reviewboard.mozilla.org/r/230948/#review236624

`CopyASCIItoUTF16`, `AppendUTF8toUTF16`, etc., currently have overloads that take a pointer to a zero-terminated string as the source argument and internally wrap it as a dependent string if it's non-null, which hides the cost of `strlen` from the caller. I'm removing these overloads and making the source argument a `Span` which XPCOM strings autoconvert to cheaply. If the source argument is `nullptr`, the destination is just truncated--i.e. the semantics of a `nullptr` source are those of an empty string. See e.g. https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#116

If `MakeStringSpan` doesn't treat a null argument as an empty span, all the call sites need to be carefully audited or made even more verbose than just adding `MakeStringSpan`, which makes the cost of `strlen` visible. Based on unit test segfaults, some of them indeed do rely on the ability to pass `nullptr`.

Since the `Span` construction from pointer and length null-checks the pointer anyway, adding a null check in `MakeStringSpan` where the constructor gets inlined into shouldn't increase the actual number of null checks done.

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230948/diff/1-2/

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

https://reviewboard.mozilla.org/r/230948/#review236624

> Two things:
> 
> 1. This behavior should be documented.  (The behavior is convenient, but it's not the universal behavior for zero-terminated string functions; e.g. `strlen` does not handle null input well.)
> 2. This behavior needs explicit tests.

Documented and tested.

> Same comments apply here as well.

Documented and tested.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230948/diff/2-3/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

https://reviewboard.mozilla.org/r/230948/#review236624

Thank you for the longer explanation.

I think this is one of those cases where the difference between `nullptr` and `"\0"` comes into play: `CopyASCIItoUTF16` is able to notice a `nullptr` argument, and not do any actual work, whereas the proposed change would wind up doing work regardless of `nullptr`-ness, even if the end result is the same.  Does this matter much?  Probably not.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8962087 [details]
Bug 1448591 - Make MakeStringSpan(nullptr) return an empty span. .

https://reviewboard.mozilla.org/r/230948/#review238916

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fb1569b61d2
Make MakeStringSpan(nullptr) return an empty span. r=froydnj.

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1fb1569b61d2