[base] Introduce base::apple::NSDataWithBytes() [chromium/src : main]
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
+avi as someone else who would be interested in this and who might have an opinion.
inline NSData* NSDataWithBytes(base::span<const uint8_t> data) {Mark MentovaiMost of the surrounding functions are all named "FooToBar". Should this follow that pattern?
Most of the surrounding functions are all named "FooToBar". Should this follow that pattern?
I think this didn’t exist before because it’s very easy to write. In addition, unlike the functions above, there’s nothing compile-time UNSAFE about this operation that demands a wrapper.
A little codesearching shows that we often do this for strings, too.
Given that the functions above AND BELOW call this a “Span”, we should probably keep the naming consistent.
If you’re adding this, you’d probably also want a mutable variant (see above and below as well).
Related details
- Mikel Astiz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
inline NSData* NSDataWithBytes(base::span<const uint8_t> data) {Mark MentovaiMost of the surrounding functions are all named "FooToBar". Should this follow that pattern?
Most of the surrounding functions are all named "FooToBar". Should this follow that pattern?
I think this didn’t exist before because it’s very easy to write. In addition, unlike the functions above, there’s nothing compile-time UNSAFE about this operation that demands a wrapper.
A little codesearching shows that we often do this for strings, too.
Given that the functions above AND BELOW call this a “Span”, we should probably keep the naming consistent.
If you’re adding this, you’d probably also want a mutable variant (see above and below as well).
Most of the surrounding functions are all named "FooToBar". Should this follow that pattern?
I think this didn’t exist before because it’s very easy to write. In addition, unlike the functions above, there’s nothing compile-time UNSAFE about this operation that demands a wrapper.
Let me know if there's no appetite for this function and I don't mind abandoning this patch.
A little codesearching shows that we often do this for strings, too.
Even if this patch, there are a number of occurrences that start with strings, which then need to use `base::as_byte_span`. I can look into another function for extra convenience, either in this patch or in a follow-up (please advise).
Given that the functions above AND BELOW call this a “Span”, we should probably keep the naming consistent.
The reason I didn't add this in the name is:
1. Most containers convert implicitly to `span`, so from the caller's standpoint `span` isn't particularly relevant (as opposed to the API signatures where `span` is returned).
2. I thought it would be nice to keep the name similar to `dataWithBytes`, so it reads familiar.
That said, I am not feeling strongly and open to any name you suggest.
If you’re adding this, you’d probably also want a mutable variant (see above and below as well).
I don't mind doing that in a follow-up (as this semi-automated CL is already large).
Related details
- Lei Zhang
- Mark Mentovai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
I prepared another patch (parent patch) targeting an API for strings specifically.
>
> Given that the functions above AND BELOW call this a “Span”, we should probably keep the naming consistent.The reason I didn't add this in the name is:
1. Most containers convert implicitly to `span`, so from the caller's standpoint `span` isn't particularly relevant (as opposed to the API signatures where `span` is returned).2. I thought it would be nice to keep the name similar to `dataWithBytes`, so it reads familiar.
That said, I am not feeling strongly and open to any name you suggest.
If you’re adding this, you’d probably also want a mutable variant (see above and below as well).I don't mind doing that in a follow-up (as this semi-automated CL is already large).
Looking for feedback whether or not this patch is worth pursuing, and if yes what the names should be.
Related details
- Lei Zhang
- Mark Mentovai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Looking for feedback whether or not this patch is worth pursuing, and if yes what the names should be.
- Lei Zhang
- Mikel Astiz
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
Looking for feedback whether or not this patch is worth pursuing, and if yes what the names should be.
I don’t know who you’re asking.
Yeah, sorry, I honestly don't know who I should be asking or what the decision process is.
I gave you my feedback.
What I understood so far:
1. The benefits of the function appear questionable, because "there’s nothing compile-time UNSAFE about this operation that demands a wrapper" --> from this PoV it seems like you're not very keen on introducing this function.
2. We should add "span" to the name for consistency with other functions --> I made a counter argument about this (but not feeling strongly). It would be nice to hear your opinion about this, but also moot if the function shouldn't be introduced to begin with so let's clarify that first.
@tse...@chromium.org as the assignee for the linked bug (which I thought was appropriate): do you have an opinion about point 1 above? There's a handful of `reinterpret_cast` and `sizeof` removed in this patch, but I don't know if this justifies the introduction of a helper function as Mark pointed out above. See also the parent patch which does the same for strings.
If there's no appetite for this, I am totally fine abandoning these two patches. What motivated my contribution is that the conversion is particularly annoying if you need to invoke a function that returns a span, e.g. `Pickle::AsBytes()`, which needs to be invoked twice or use a local variable.
Related details
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
I’m lukewarm. There’s no strong reason to add it, but I’m not against it, either. If people like it because there’s an interface at this level to perform the inverse operation, that’s fine with me. I can absolutely see an argument in favor of doing this on that basis.
2. We should add "span" to the name for consistency with other functions --> I made a counter argument about this (but not feeling strongly). It would be nice to hear your opinion about this, but also moot if the function shouldn't be introduced to begin with so let's clarify that first.
If you do add it, it’s solely in service of point 1 above. If you have an interface at this level to perform the inverse of an operation that’s already there, they need to use common language. You might like “bytes” naming better, but that’s not relevant when the existing code uses “span” naming. In fact, I think I like “bytes” naming better too, but my opinion on this point is just as irrelevant when there’s an established practice already in place.
If you felt strongly enough about the naming, you’d need to rename the existing uses too. I’m also lukewarm about this, but if you wanted to do it, it’s also fine with me.
“Handle mutable” is similar. The existing interface already handles immutable and mutable variants. If you’re adding to that existing body of work, you need to play by the rules that it’s already established. I don’t really care if you do it in one change or two, but I’d want to at least review them both at the same time, because a systematic problem that arises in one might have an impact on the other. At that point, you’re bumping up against merge conflict territory anyway, so you might as well load the work up into a single change. But I wouldn’t insist. If you like it better as two simultaneous changes and don’t mind merge toil, that’s fine.
@tse...@chromium.org as the assignee for the linked bug (which I thought was appropriate): do you have an opinion about point 1 above? There's a handful of `reinterpret_cast` and `sizeof` removed in this patch, but I don't know if this justifies the introduction of a helper function as Mark pointed out above. See also the parent patch which does the same for strings.
If there's no appetite for this, I am totally fine abandoning these two patches. What motivated my contribution is that the conversion is particularly annoying if you need to invoke a function that returns a span, e.g. `Pickle::AsBytes()`, which needs to be invoked twice or use a local variable.
I also asked @a...@chromium.org for his opinion. He also uses and works with this code regularly.
Related details
- Mikel Astiz
- Tom Sepez
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
I find it difficult to find enthusiasm for this change.
This replaces a single message send with a one-liner wrapper function. The convenience factory method `+[NSData dataWithBytes:length:]` is core API that folks that use Foundation are already very familiar with, and this change now expects them to switch to using a wrapper that they wouldn’t necessarily expect to exist.
We have wrappers that are one-liner-for-one-liner replacements (`(__bridge <<type>>)` vs `base::apple::CFToNSPtrCast<>`). However, for those there is some additional advantage gained by using them, whether doing additional type-checking or centralizing unsafe behavior, but I don’t see the advantage in this case.
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
Thanks Avi, that's reasonable.
One question though regarding unsafe behavior: we are often required to annotate with `UNSAFE_BUFFERS` when dealing with pointer/size pairs. I'm not sure what the exact criteria are, but `base::span_from_ref()` for example internally uses `UNSAFE_BUFFERS` when constructing a span. It strikes me as suspicious that `dataWithBytes` is considered safer:
```
NSData* data = [NSData dataWithBytes:buffer length:sizeof(buffer)]; // All good.
span<const uint8_t> span = NSDataToSpan(data); // All good.
span<const uint8_t> span2(buffer, sizeof(buffer)); // error: function introduces unsafe buffer manipulation [-Werror,-Wunsafe-buffer-usage]
```
Should this affect the fate of this patch? Or would it for example only make sense in combination with `UNSAFE_BUFFERS` being enforced for direct calls to `dataWithBytes`?
Again, to be clear, I don't want to make a strong case for introducing these APIs. Just making sure it is fully thought through, now that there's attention from several owners.
Related details
- Avi Drissman
- Tom Sepez
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
I am in agreement that `+[NSData dataWithBytes:length:]` is not safe. But this is at a platform API boundary, so the virality of spanification has to stop there.
Have we taken action at other platform API boundaries, and if so, is there precedent to refer to?
Related details
- Mikel Astiz
- Tom Sepez
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Other platforms? No, but other platforms don't have a platform-specific representation of owned ptr/length pairs -- just discrete arguments to system calls, say. So you're on our own here. Are there APIs on Mac that take a NSData instead of a pointer and length? If so, then this seems reasonable.
Related details
- Mikel Astiz
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
Most every API on the Mac that requires “a bunch of bytes” uses `NSData`. It’s the creation of the `NSData` itself that requires pointer/length.
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
So where I might imagine this as being useful is to write
`NSDataWithBytes(data.subspan(10, 20))` instead of `[NSData dataWithBytes:data.data() + 10, length:20]` or
```
auto sub_data = data.subspan(10, 20);
[NSData dataWithBytes:data.sub_data() length:sub_data.size()];
```