spanification: automatically spanify .../common/partial_circular_buffer.cc etc. [chromium/src : main]
Stephen Nusko (Gerrit)
Stephen Nusko added 5 comments![]( "Open in Gerrit")
void Write(const void* buffer, uint32_t buffer_size);Should we do Write while we are here to keep this file consistent?
uint32_t header_size = offsetof(BufferData, data);Is there any concerns about compatibility?
I.E. is it possible that a `BufferData` struct was written by a part of the binary that has a different definition?
If so offsetof could be incorrect here.
If there is none then obviously this is better (it is compile time)
if (total_read_ >= buffer_data_->total_written)does a base::spanWriter make sense here? Not entirely sure but seems similar.
// uint8_t* buffer_uint8 = reinterpret_cast<uint8_t*>(buffer);commented out code should be removed.
UNSAFE_TODO(base::span(buffer_data_->data + position_, to_read)));Should we create at the top of this a proper base::span that represents all the valid memory? And then this just copies the relevant subspan?
Then we can add a
UNSAFE_TODO in a single location.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Tomas Gunnarsson
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 5 comments![]( "Open in Gerrit")
Should we do Write while we are here to keep this file consistent?
Done
uint32_t header_size = offsetof(BufferData, data);Is there any concerns about compatibility?
I.E. is it possible that a `BufferData` struct was written by a part of the binary that has a different definition?
If so offsetof could be incorrect here.
If there is none then obviously this is better (it is compile time)
Given all the uses of PartialCircularBuffer [https://source.chromium.org/search?q=PartialCircularBuffer&sq=] it seems they always use the same BufferData definition.
if (total_read_ >= buffer_data_->total_written)does a base::spanWriter make sense here? Not entirely sure but seems similar.
we cannot add a SpanWriter as class member since it must be stack allocated :(
// uint8_t* buffer_uint8 = reinterpret_cast<uint8_t*>(buffer);commented out code should be removed.
Done
UNSAFE_TODO(base::span(buffer_data_->data + position_, to_read)));Should we create at the top of this a proper base::span that represents all the valid memory? And then this just copies the relevant subspan?
Then we can add a
UNSAFE_TODO in a single location.
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
- Tomas Gunnarsson
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 5 comments![]( "Open in Gerrit")
circular_(&buffer_[0], sizeof(buffer_), sizeof(buffer_) / 2, false),The compiler is complaining that this isn't the correct format for the PartialCircularBuffer constructor.
uint32_t memory_buffer_size_;
uint32_t data_size_;Perhaps these should be removed as well? Or at least one of them?
raw_ptr<BufferData, DanglingUntriaged> buffer_data_;You should be able to remove `buffer_data_` if you have the span?
PartialCircularBuffer(base::span<uint8_t> buffer);Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
std::vector<uint8_t> buffer_;The replacement for an std::unique_ptr<uint8_t[]> is a base;:HeapArray<uint8_t>
why? Because we don't need the logic about dynamically growing the std::vector.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Tomas Gunnarsson
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 4 comments![]( "Open in Gerrit")
circular_(&buffer_[0], sizeof(buffer_), sizeof(buffer_) / 2, false),The compiler is complaining that this isn't the correct format for the PartialCircularBuffer constructor.
Done
Perhaps these should be removed as well? Or at least one of them?
Done
raw_ptr<BufferData, DanglingUntriaged> buffer_data_;You should be able to remove `buffer_data_` if you have the span?
I think we cannot remove it yet, because the fields of BufferData are being used in several places within this class. And if we remove it, we would need to cast buffer_data_span_ everytime we access the fields of BufferData.
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
- Tomas Gunnarsson
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
std::vector<uint8_t> buffer_;The replacement for an std::unique_ptr<uint8_t[]> is a base;:HeapArray<uint8_t>
why? Because we don't need the logic about dynamically growing the std::vector.
Done
Related details
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
- Tomas Gunnarsson
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. |
Tomas Gunnarsson (Gerrit)
Tomas Gunnarsson voted Code-Review+1![]( "Open in Gerrit")
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
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. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
3 comments
raw_ptr<BufferData, DanglingUntriaged> buffer_data_;Bryan Enrique GonzalezYou should be able to remove `buffer_data_` if you have the span?
I think we cannot remove it yet, because the fields of BufferData are being used in several places within this class. And if we remove it, we would need to cast buffer_data_span_ everytime we access the fields of BufferData.
Fair enough a bit unfortunate that we need to pay the raw_ptr cost twice.
uint32_t header_size = offsetof(BufferData, data);Bryan Enrique GonzalezIs there any concerns about compatibility?
I.E. is it possible that a `BufferData` struct was written by a part of the binary that has a different definition?
If so offsetof could be incorrect here.
If there is none then obviously this is better (it is compile time)
Given all the uses of PartialCircularBuffer [https://source.chromium.org/search?q=PartialCircularBuffer&sq=] it seems they always use the same BufferData definition.
Assuming there isn't anything else, but guess we'll find out when submitting 😊
if (total_read_ >= buffer_data_->total_written)Bryan Enrique Gonzalezdoes a base::spanWriter make sense here? Not entirely sure but seems similar.
we cannot add a SpanWriter as class member since it must be stack allocated :(
Acknowledged
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |