spanification: automatically spanify ui/gfx/linux/client_native_pixmap_dmabuf.cc etc. [chromium/src : main]
Roberto Torres (Gerrit)
Roberto Torres voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Colin Blundell
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
size_t size = 0;Add a comment here explaining size is the size requested for `data`, I.E. the reason it says is PlaneInfo is constructed with only `offset` and `size` set and then later `data` is allocated with that information.
Basically I was confused if we could remove this (answer is no), because of the above. Feel free to reword but this should have a clarification comment.
base::raw_span<uint8_t> data;Also this seems strange to be a raw_span/raw_ptr... It is owned by PlaneInfo, deleted in its constructor.
What do you think about
```
base::HeapArray<uint8_t> data;
```
And using [FromOwningPointer](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/heap_array.h;l=102;drc=a1d268ec0bc5f1b763bd8dfd373264a4d57ef00b) with a custom deleting_type that calls `munmap`?
Then the ownership is clearer, we don't need raw_span (avoiding the overhead of raw_ptr), and we still get bounds checking and .AsSpan() functions for safe access.
// `size_to_map` bytes. // Set nullptr to info.data in order not to call munmap in |info| dtor.outdated comment
Related details
- Colin Blundell
- Roberto Torres
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. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! Please add me back once Stephen has LGTM'd.
Related details
- Roberto Torres
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. |
Roberto Torres (Gerrit)
Roberto Torres added 4 comments![]( "Open in Gerrit")
Add a comment here explaining size is the size requested for `data`, I.E. the reason it says is PlaneInfo is constructed with only `offset` and `size` set and then later `data` is allocated with that information.
Basically I was confused if we could remove this (answer is no), because of the above. Feel free to reword but this should have a clarification comment.
Done
Also this seems strange to be a raw_span/raw_ptr... It is owned by PlaneInfo, deleted in its constructor.
What do you think about
```
base::HeapArray<uint8_t> data;
```And using [FromOwningPointer](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/heap_array.h;l=102;drc=a1d268ec0bc5f1b763bd8dfd373264a4d57ef00b) with a custom deleting_type that calls `munmap`?
Then the ownership is clearer, we don't need raw_span (avoiding the overhead of raw_ptr), and we still get bounds checking and .AsSpan() functions for safe access.
Marked as resolved.
// `size_to_map` bytes. // Set nullptr to info.data in order not to call munmap in |info| dtor.Roberto Torresoutdated comment
Done
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
plane_info_[plane].data.subspan(plane_info_[plane].offset).data());can this subspan become an index acces + & operator?
Related details
- Roberto Torres
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. |
Roberto Torres (Gerrit)
Roberto Torres voted and added 1 comment![]( "Open in Gerrit")
Votes added by Roberto Torres
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
plane_info_[plane].data.subspan(plane_info_[plane].offset).data());can this subspan become an index acces + & operator?
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
I'll review after Stephen LGTM's, thanks!
Related details
- Roberto Torres
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Roberto Torres
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Thanks! LGTM for OWNERS, defer to Stephen on the technical review.
Related details
- Roberto Torres
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
spanification: automatically spanify ui/gfx/linux/client_native_pixmap_dmabuf.cc etc.
This is the result of running the automatic spanification on linux and
updating code to use and pass spans where size is known.
The original patch was fully automated using script:
//tools/clang/spanify/rewrite-multiple-platforms.sh -platforms=linux
Then refined with gemini-cli
gemini-run/batch-run-1761798078/group_87
BUG=439964610
- M ui/gfx/linux/client_native_pixmap_dmabuf.cc
- M ui/gfx/linux/client_native_pixmap_dmabuf.h
Code-Review: +1 by Stephen Nusko, +1 by Colin Blundell
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |