[base] Spanify Pickle's exposure of underlying data [chromium/src : main]
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
1 comment
dcheng@: can you take a look at this change, starting with base/pickle.h?
I noticed the current APIs are not spanified and decided to take a stab. It's possible that I need to update a few additional occurrences, but it builds locally at least.
If the direction is looking good, I would also like to hear your opinion on whether or not this patch is worth splitting into smaller ones. That would involve temporarily exposing both APIs, hence coming up with a new name (e.g. `data_as_bytes()`).
Related details
- Daniel Cheng
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")
// which in turn allow it to be implicitly converted to a `span`.dch...@chromium.org: one option would be to remove begin() and end() functions altogether, and let callers migrate to using Pickle::data().
Related details
- Daniel Cheng
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
dcheng@: can you take a look at this change, starting with base/pickle.h?
I noticed the current APIs are not spanified and decided to take a stab. It's possible that I need to update a few additional occurrences, but it builds locally at least.
If the direction is looking good, I would also like to hear your opinion on whether or not this patch is worth splitting into smaller ones. That would involve temporarily exposing both APIs, hence coming up with a new name (e.g. `data_as_bytes()`).
Upon second look at this, I am leaning towards renaming the functions anyway, as proposed in the latest patchset (precise names TBD).
Some of changes on the caller's side are non-trivial so I am also thinking they should be split to dedicated patches. I also don't know where to stop in the rabbit hole, so it seems better to discuss those cases specifically (e.g. I ended up refactoring `base/feature_list.cc` more than originally planned).
Nevertheless, I would appreciate early feedback on the proposed API and naming, before we discuss the details of calling sites and how the changes should be split.
if (pickle.AsBytes().empty() || pickle.payload_size() < sizeof(Header)) {The second OR operand should be reverted to the original expression.
Related details
- Daniel Cheng
- 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. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
class FeatureEntry {This probably overlaps with https://chromium-review.googlesource.com/c/chromium/src/+/6813554.
Please coordinate with the other change to figure out what's the right approach/path forward. I didn't review in details.
Related details
- Daniel Cheng
- 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")
class FeatureEntry {This probably overlaps with https://chromium-review.googlesource.com/c/chromium/src/+/6813554.
Please coordinate with the other change to figure out what's the right approach/path forward. I didn't review in details.
Thanks for flagging. This patch is currently seeking preliminary review only, so I would sort out conflicts later (and in split form).
Related details
- Daniel Cheng
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 2 comments![]( "Open in Gerrit")
// which in turn allow it to be implicitly converted to a `span`.dch...@chromium.org: one option would be to remove begin() and end() functions altogether, and let callers migrate to using Pickle::data().
data() was renamed to AsBytes() in the latest versions.
There's a broader topic here though: currently Pickle can be converted implicitly to a span, and I question if this is desirable. To me, this class is a somewhat complex datatype and an implicit conversion is worse than explicitly having to call AsBytes(). So I suggest either of two options:
1) [My preference] Remove implicit conversion to span (which implies removing begin()/end() APIs).
2) Remove AsBytes() and migrate callers to implicit conversion to span.
if (pickle.AsBytes().empty() || pickle.payload_size() < sizeof(Header)) {The second OR operand should be reverted to the original expression.
Done
Related details
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
// which in turn allow it to be implicitly converted to a `span`.Mikel Astizdch...@chromium.org: one option would be to remove begin() and end() functions altogether, and let callers migrate to using Pickle::data().
data() was renamed to AsBytes() in the latest versions.
There's a broader topic here though: currently Pickle can be converted implicitly to a span, and I question if this is desirable. To me, this class is a somewhat complex datatype and an implicit conversion is worse than explicitly having to call AsBytes(). So I suggest either of two options:
1) [My preference] Remove implicit conversion to span (which implies removing begin()/end() APIs).
2) Remove AsBytes() and migrate callers to implicit conversion to span.
I think I agree; having `base::Pickle` be implicitly spannable feels a bit weird to me. I agree with removing `begin()` and `end()` and having explicit helpers to get the contents as a span.
The proposed names above seem fine to me; personally, I would probably just have `AsBytes()` and `AsWritableBytes()`; if people want as_string_view(), they can always manually call it themselves right? (I don't feel strongly about this last point, but I would try to avoid exposing APIs that expose arbitrary binary data as string views when possible. Obviously... people can still do it, but I'd still feel better about it :)
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")
// which in turn allow it to be implicitly converted to a `span`.Mikel Astizdch...@chromium.org: one option would be to remove begin() and end() functions altogether, and let callers migrate to using Pickle::data().
Daniel Chengdata() was renamed to AsBytes() in the latest versions.
There's a broader topic here though: currently Pickle can be converted implicitly to a span, and I question if this is desirable. To me, this class is a somewhat complex datatype and an implicit conversion is worse than explicitly having to call AsBytes(). So I suggest either of two options:
1) [My preference] Remove implicit conversion to span (which implies removing begin()/end() APIs).
2) Remove AsBytes() and migrate callers to implicit conversion to span.
I think I agree; having `base::Pickle` be implicitly spannable feels a bit weird to me. I agree with removing `begin()` and `end()` and having explicit helpers to get the contents as a span.
The proposed names above seem fine to me; personally, I would probably just have `AsBytes()` and `AsWritableBytes()`; if people want as_string_view(), they can always manually call it themselves right? (I don't feel strongly about this last point, but I would try to avoid exposing APIs that expose arbitrary binary data as string views when possible. Obviously... people can still do it, but I'd still feel better about it :)
I've split API changes to https://chromium-review.googlesource.com/c/chromium/src/+/7516607, we can continue to detailed discussion there.
Regarding string_view (but feel free to continue this thread in the other CL): I think strings are a bit special because historically they have been used to represent blobs. Even Pickle API does this internally, and several callers (including proto-generated code) still use std::string to represent blob data. That said, I don't feel particularly strongly about adding this API.
Related details
- Daniel Cheng
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")
// which in turn allow it to be implicitly converted to a `span`.Mikel Astizdch...@chromium.org: one option would be to remove begin() and end() functions altogether, and let callers migrate to using Pickle::data().
Daniel Chengdata() was renamed to AsBytes() in the latest versions.
There's a broader topic here though: currently Pickle can be converted implicitly to a span, and I question if this is desirable. To me, this class is a somewhat complex datatype and an implicit conversion is worse than explicitly having to call AsBytes(). So I suggest either of two options:
1) [My preference] Remove implicit conversion to span (which implies removing begin()/end() APIs).
2) Remove AsBytes() and migrate callers to implicit conversion to span.
Mikel AstizI think I agree; having `base::Pickle` be implicitly spannable feels a bit weird to me. I agree with removing `begin()` and `end()` and having explicit helpers to get the contents as a span.
The proposed names above seem fine to me; personally, I would probably just have `AsBytes()` and `AsWritableBytes()`; if people want as_string_view(), they can always manually call it themselves right? (I don't feel strongly about this last point, but I would try to avoid exposing APIs that expose arbitrary binary data as string views when possible. Obviously... people can still do it, but I'd still feel better about it :)
I've split API changes to https://chromium-review.googlesource.com/c/chromium/src/+/7516607, we can continue to detailed discussion there.
Regarding string_view (but feel free to continue this thread in the other CL): I think strings are a bit special because historically they have been used to represent blobs. Even Pickle API does this internally, and several callers (including proto-generated code) still use std::string to represent blob data. That said, I don't feel particularly strongly about adding this API.
I ended up looking more closely into base::Pickle and decided to engage a bit more, specially after noticing how odd the instances with unowned buffers are.
I captured a proposal in https://docs.google.com/document/d/1cJXxEk0DwzRqxmXd65KZhpeBHmMP5ZShO7LJUFdZhZw/edit?usp=sharing.
dch...@chromium.org would you mind taking a look and sharing if this is worth pursuing and whose approval it would require? I'm prototyped most of the work and things seem to be feasible. Among other things, the IPC code would need some mechanical changes that would need review from IPC/Mojo owners, but I notice you happen to be listed as one.
Related details
- Daniel Cheng
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. |