spanification: automatically spanify remoting/protocol/pseudo_tcp.cc etc. [chromium/src : main]
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Arthur Sonzogni
- 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 15 comments![]( "Open in Gerrit")
std::unique_ptr<char[]> buffer_ GUARDED_BY(lock_);While we are here switch this to a base::HeapArray? and remove `buffer_length_`? (still need data_length_ most likely).
auto bytes = base::U32ToBigEndian(val);Because this is network code might be best to do
`base::as_bytes(base::HostToNet32(val));`
Thoughts? Here and below?
I agree that new is BigEndian so that is fine, but they have a nice helper function and it probably makes more sense to keep it consistent?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))We can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
const uint8_t* Data() const { return data_.subspan(pos_).data(); }`&data_[pos_]`
is better I think?
// SAFETY: `buffer` is a raw C-style array of size `len`, so we can safely
// reinterpret the pointer as a span of bytes.The safety concern isn't from the reinterpret it is from no knowing if the length is correct. Same as above we can't use UNSAFE_BUFFERS here.
std::unique_ptr<uint8_t[]> buffer(new uint8_t[MAX_PACKET]);Switch this to a HeapArray then you can get a span directly.
buffer_span.first<4u>().copy_from(base::U32ToBigEndian(m_conv));
buffer_span.subspan(4u).first<4u>().copy_from(base::U32ToBigEndian(seq));
buffer_span.subspan(8u).first<4u>().copy_from(
base::U32ToBigEndian(m_rcv_nxt));
buffer_span[12] = 0;
buffer_span[13] = flags;
buffer_span.subspan(14u).first<2u>().copy_from(
base::U16ToBigEndian(static_cast<uint16_t>(m_rcv_wnd >> m_rwnd_scale)));
// Timestamp computations
buffer_span.subspan(16u).first<4u>().copy_from(base::U32ToBigEndian(now));
buffer_span.subspan(20u).first<4u>().copy_from(
base::U32ToBigEndian(m_ts_recent));This looks like a SpanWriter?
seg.conv = base::U32FromBigEndian(buffer.first<4u>());
seg.seq = base::U32FromBigEndian(buffer.subspan(4u).first<4u>());
seg.ack = base::U32FromBigEndian(buffer.subspan(8u).first<4u>());
seg.flags = buffer[13];
seg.wnd = base::U16FromBigEndian(buffer.subspan(14u).first<2u>());
seg.tsval = base::U32FromBigEndian(buffer.subspan(16u).first<4u>());
seg.tsecr = base::U32FromBigEndian(buffer.subspan(20u).first<4u>());This looks like a spanReader?
} else if (seg.data.size() != 0) {nit: !seq.data.empty()
seg.data = seg.data.first(seg.data.size() - nAdjust);I think you are double moving the buffer here?
```
seg.data += nAdjust;
seg.len -= nAdjust;
```
The old code moved the buffer and then decreased the length by the same amount (to keep them in sync).
Your new code moves the buffer forward by nAdjust and then takes a further nAdjust off in addition (this is incorrect).
Delete the seq.data.first line?
if (!m_rbuf.WriteOffset(seg.data.data(), seg.data.size(), nOffset,Can we update WriteOffset?
bool PseudoTcp::LockedFifoBuffer::WriteOffset(const void* buffer,
size_t bytes,It looks like we call this function with a span could we update this one?
bool PseudoTcp::LockedFifoBuffer::Read(void* buffer,
size_t bytes,Can we check all of these?
dest(reinterpret_cast<char*>(buffer.data()), buffer.size()));base::as_writable_chars(buffer) should work no?
base::span<const char> UNSAFE_BUFFERS(
src(&buffer_[read_position], tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> UNSAFE_BUFFERS(src2(&buffer_[0], copy - tail_copy));base::as_chars?
Related details
- Arthur Sonzogni
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 15 comments![]( "Open in Gerrit")
While we are here switch this to a base::HeapArray? and remove `buffer_length_`? (still need data_length_ most likely).
Done
Because this is network code might be best to do
`base::as_bytes(base::HostToNet32(val));`
Thoughts? Here and below?
I agree that new is BigEndian so that is fine, but they have a nice helper function and it probably makes more sense to keep it consistent?
Done
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))We can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
I was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
const uint8_t* Data() const { return data_.subspan(pos_).data(); }`&data_[pos_]`
is better I think?
Done
// SAFETY: `buffer` is a raw C-style array of size `len`, so we can safely
// reinterpret the pointer as a span of bytes.The safety concern isn't from the reinterpret it is from no knowing if the length is correct. Same as above we can't use UNSAFE_BUFFERS here.
Done
std::unique_ptr<uint8_t[]> buffer(new uint8_t[MAX_PACKET]);Switch this to a HeapArray then you can get a span directly.
Done
buffer_span.first<4u>().copy_from(base::U32ToBigEndian(m_conv));
buffer_span.subspan(4u).first<4u>().copy_from(base::U32ToBigEndian(seq));
buffer_span.subspan(8u).first<4u>().copy_from(
base::U32ToBigEndian(m_rcv_nxt));
buffer_span[12] = 0;
buffer_span[13] = flags;
buffer_span.subspan(14u).first<2u>().copy_from(
base::U16ToBigEndian(static_cast<uint16_t>(m_rcv_wnd >> m_rwnd_scale)));
// Timestamp computations
buffer_span.subspan(16u).first<4u>().copy_from(base::U32ToBigEndian(now));
buffer_span.subspan(20u).first<4u>().copy_from(
base::U32ToBigEndian(m_ts_recent));This looks like a SpanWriter?
Done
seg.conv = base::U32FromBigEndian(buffer.first<4u>());
seg.seq = base::U32FromBigEndian(buffer.subspan(4u).first<4u>());
seg.ack = base::U32FromBigEndian(buffer.subspan(8u).first<4u>());
seg.flags = buffer[13];
seg.wnd = base::U16FromBigEndian(buffer.subspan(14u).first<2u>());
seg.tsval = base::U32FromBigEndian(buffer.subspan(16u).first<4u>());
seg.tsecr = base::U32FromBigEndian(buffer.subspan(20u).first<4u>());This looks like a spanReader?
Done
} else if (seg.data.size() != 0) {Daniel Angulonit: !seq.data.empty()
Done
I think you are double moving the buffer here?
```
seg.data += nAdjust;
seg.len -= nAdjust;
```The old code moved the buffer and then decreased the length by the same amount (to keep them in sync).
Your new code moves the buffer forward by nAdjust and then takes a further nAdjust off in addition (this is incorrect).
Delete the seq.data.first line?
Done
if (!m_rbuf.WriteOffset(seg.data.data(), seg.data.size(), nOffset,Daniel AnguloCan we update WriteOffset?
Done
bool PseudoTcp::LockedFifoBuffer::WriteOffset(const void* buffer,
size_t bytes,It looks like we call this function with a span could we update this one?
Done
bool PseudoTcp::LockedFifoBuffer::Read(void* buffer,
size_t bytes,Can we check all of these?
Done
dest(reinterpret_cast<char*>(buffer.data()), buffer.size()));base::as_writable_chars(buffer) should work no?
Done
base::span<const char> UNSAFE_BUFFERS(
src(&buffer_[read_position], tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> UNSAFE_BUFFERS(src2(&buffer_[0], copy - tail_copy));- Arthur Sonzogni
- 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 added 7 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
I was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
If this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(buffer, len))),We can't use UNSAFE_BUFFERS here still, we have the same issue as the other location.
uint32_t PseudoTcp::queue(const char* data, uint32_t len, bool bCtrl) {Can we make this a span as well?
reader.Skip(1u);What is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
parseOptions(seg.data.subspan(1u));nit: template subspan<1u>
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));could this be written as a base::span::split? I think so?
base::span<char> dest = base::as_writable_chars(buffer);
base::span<const char> src =
base::as_chars(buffer_.subspan(read_position, tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> src2 =
base::as_chars(buffer_.subspan(0, copy - tail_copy));
dest.subspan(tail_copy, copy - tail_copy).copy_from(src2);I didn't fully math this out but also feels like a split of some sort?
Related details
- Arthur Sonzogni
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 7 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
If this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
isn't it writing data+pos into the same str that is the parameter?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(buffer, len))),We can't use UNSAFE_BUFFERS here still, we have the same issue as the other location.
Done
uint32_t PseudoTcp::queue(const char* data, uint32_t len, bool bCtrl) {Can we make this a span as well?
Done
reader.Skip(1u);What is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
in the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
parseOptions(seg.data.subspan(1u));Daniel Angulonit: template subspan<1u>
Done
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));could this be written as a base::span::split? I think so?
Done
base::span<char> dest = base::as_writable_chars(buffer);
base::span<const char> src =
base::as_chars(buffer_.subspan(read_position, tail_copy));
dest.first(tail_copy).copy_from(src);
base::span<const char> src2 =
base::as_chars(buffer_.subspan(0, copy - tail_copy));
dest.subspan(tail_copy, copy - tail_copy).copy_from(src2);I didn't fully math this out but also feels like a split of some sort?
- Arthur Sonzogni
- 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 added 11 comments![]( "Open in Gerrit")
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
isn't it writing data+pos into the same str that is the parameter?
Obviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.No need for a safety comment.
base::span<uint8_t> buffer(buffer_storage);You don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
in the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Perfect can you add a comment mentioning this? Makes sense.
// SAFETY: buf.Data() is a pointer to uint8_t of size buf.Length(), and we
// cast it to a pointer to char of that size.
queue(UNSAFE_BUFFERS(base::span(reinterpret_cast<const char*>(buf.Data()),
static_cast<uint32_t>(buf.Length()))),We own the ByteBufferWriter type here in our code, and are already modifying it. Can we just add a data() and size() method to it so it can directly construct a span and then we can use `as_writable_bytes`?
Then remove the SAFETY comment.
new_buffer_first.copy_from(buffer_.subspan(read_position_, tail_copy));copy_from_nonoverlapping (these are separate heap arrays they can't be the same memory).
const size_t tail_copy = std::min(copy, buffer_.size() - write_position);
buffer_.subspan(write_position, tail_copy)
.copy_from(base::as_chars(buffer.first(tail_copy)));
buffer_.first(copy - tail_copy)
.copy_from(base::as_chars(buffer.subspan(tail_copy, copy - tail_copy)));Switch this into a split as well?
UNSAFE_BUFFERS(base::span(packet.c_str(), packet.size())));We don't need this UNSAFE_BUFFERS
Just make a string_view or use `base::as_writable_chars`
received = remote_.Recv(base::span(block).first(block.size()));first(block.size) of a span of block doesn't make any difference. Remove .first()
received = receiver_->Recv(base::span(block).first(block.size()));Also review this and other calls for redundant first calls.
base::span(buffer->data(), static_cast<size_t>(buffer_size))));Can't you juse use `buffer.span()`? here and below?
Related details
- Arthur Sonzogni
- Daniel Angulo
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Thanks!
(I am waiting for the current suggestion to be addressed + the build to pass)
Feel free to close this comment when this patch is ready to be reviewed again.
Related details
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 13 comments![]( "Open in Gerrit")
Thanks!
(I am waiting for the current suggestion to be addressed + the build to pass)
Feel free to close this comment when this patch is ready to be reviewed again.
Done
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
Stephen Nuskoisn't it writing data+pos into the same str that is the parameter?
Obviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
base::as_writable_byte_span cannot be used with a std::string_view. Here's why
std::string_view is Read-Only: A std::string_view (and absl::string_view) provides a non-owning, read-only view into an existing character sequence. It doesn't own the data and cannot be used to modify it.
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.No need for a safety comment.
Done
You don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
no viable constructor or deduction guide for deduction of template arguments of 'base::SpanWriter'
589 | base::SpanWriter writer(buffer_storage);
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
Stephen Nuskoin the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Perfect can you add a comment mentioning this? Makes sense.
Done
// SAFETY: buf.Data() is a pointer to uint8_t of size buf.Length(), and we
// cast it to a pointer to char of that size.
queue(UNSAFE_BUFFERS(base::span(reinterpret_cast<const char*>(buf.Data()),
static_cast<uint32_t>(buf.Length()))),We own the ByteBufferWriter type here in our code, and are already modifying it. Can we just add a data() and size() method to it so it can directly construct a span and then we can use `as_writable_bytes`?
Then remove the SAFETY comment.
Done
new_buffer_first.copy_from(buffer_.subspan(read_position_, tail_copy));copy_from_nonoverlapping (these are separate heap arrays they can't be the same memory).
Done
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));Daniel Angulocould this be written as a base::span::split? I think so?
Done
how? I see that there could be a split at tail_copy, but buffer_.subspan(0, copy - tail_copy) starts at offset 0 not at tail_copy, maybe I'm just a little bit confused but I don't see a clean split
const size_t tail_copy = std::min(copy, buffer_.size() - write_position);
buffer_.subspan(write_position, tail_copy)
.copy_from(base::as_chars(buffer.first(tail_copy)));
buffer_.first(copy - tail_copy)
.copy_from(base::as_chars(buffer.subspan(tail_copy, copy - tail_copy)));Switch this into a split as well?
Done
UNSAFE_BUFFERS(base::span(packet.c_str(), packet.size())));We don't need this UNSAFE_BUFFERS
Just make a string_view or use `base::as_writable_chars`
Done
received = remote_.Recv(base::span(block).first(block.size()));first(block.size) of a span of block doesn't make any difference. Remove .first()
Done
received = receiver_->Recv(base::span(block).first(block.size()));Also review this and other calls for redundant first calls.
Done
base::span(buffer->data(), static_cast<size_t>(buffer_size))));Can't you juse use `buffer.span()`? here and below?
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 6 comments![]( "Open in Gerrit")
int PseudoTcp::Recv(base::span<char> buffer) {Could this be uint8_t?
if (!m_rbuf.Read(base::as_writable_bytes(buffer), &read)) {This can be removed after applying my previous comment.
int buffer_size,Can we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
// SAFETY: buffer_size is the size of the buffer, and the buffer is a pointer
// to char of that size.We don't need a "safety" comment here. There are no UNSAFE_BUFERS
int result = pseudo_tcp_.Recv(base::as_writable_chars(buffer->span()));This can be removed if you apply one of my other comments about passing `base::span<uint8_t>`
Related details
- Daniel Angulo
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 5 comments![]( "Open in Gerrit")
int PseudoTcp::Recv(base::span<char> buffer) {Daniel AnguloCould this be uint8_t?
Done
if (!m_rbuf.Read(base::as_writable_bytes(buffer), &read)) {This can be removed after applying my previous comment.
Can we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
check_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
// SAFETY: buffer_size is the size of the buffer, and the buffer is a pointer
// to char of that size.We don't need a "safety" comment here. There are no UNSAFE_BUFERS
Done
int result = pseudo_tcp_.Recv(base::as_writable_chars(buffer->span()));This can be removed if you apply one of my other comments about passing `base::span<uint8_t>`
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 6 comments![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Optional: Ma
int result = pseudo_tcp_.Recv(buffer->span().first(buffer_size));This could be `first` directly.
int result = pseudo_tcp_.Send(buffer->span().first(buffer_size));this could be `first`
// SAFETY: read_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Recv(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(read_buffer_->data()),
static_cast<size_t>(read_buffer_size_))));We could avoid `UNSAFE_BUFFERS`, here because `read_buffer_->first(read_buffer_size_)` exist.
(There is also `->span()` for the full span)
// SAFETY: write_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Send(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(write_buffer_->data()),
static_cast<size_t>(write_buffer_size_))));Ditto
pseudo_tcp_.NotifyPacket(UNSAFE_BUFFERS(Ditto
Related details
- Daniel Angulo
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 7 comments![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Ma?
int result = pseudo_tcp_.Recv(buffer->span().first(buffer_size));This could be `first` directly.
Done
int result = pseudo_tcp_.Send(buffer->span().first(buffer_size));Daniel Angulothis could be `first`
Done
// SAFETY: read_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Recv(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(read_buffer_->data()),
static_cast<size_t>(read_buffer_size_))));We could avoid `UNSAFE_BUFFERS`, here because `read_buffer_->first(read_buffer_size_)` exist.
(There is also `->span()` for the full span)
Done
// SAFETY: write_buffer_size_ is the size of the buffer, and the buffer is a
// pointer to char of that size.
int result = pseudo_tcp_.Send(UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(write_buffer_->data()),
static_cast<size_t>(write_buffer_size_))));Daniel AnguloDitto
Done
pseudo_tcp_.NotifyPacket(UNSAFE_BUFFERS(Daniel AnguloDitto
Done
Related details
- Arthur Sonzogni
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Ma?
sorry, what did you say?
Related details
- Arthur Sonzogni
- 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 added 9 comments![]( "Open in Gerrit")
base::raw_span<const uint8_t> data;Do we know if there is any benchmarks around this code? This is the line most likely to trigger potential performance impact as we create and destroy segments.
void WriteString(const char* str, size_t len) {
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.
auto bytes = base::as_byte_span(UNSAFE_BUFFERS(base::span(str, len)));There is no caller of this function? Lets just switch it to a span? or delete it.
*val = base::U32FromBigEndian(data_.subspan(pos_).first<4u>());Should we continue to use base::NetToHost32? Here and below to keep the convention?
base::as_writable_bytes(UNSAFE_BUFFERS(base::span(str, len)))Daniel AnguloWe can't claim UNSAFE_BUFFERS here, what SAFETY message would you write?
We have 3 options
1) Make this function take std::string_view
2) Make this function UNSAFE_BUFFER_USAGE and force all callers to add UNSAFE_TODO or UNSAFE_BUFFERS
3) Mark this as UNSAFE_TODO for now until someone can do 1 or 2.
Stephen NuskoI was thinking that string_view can't be used because it needs to write data on it, but making this function take a span<char> would work instead. However this function is not used anywhare, should I stray it?
Daniel AnguloIf this function isn't used we should just delete it I think?
I guess the API it is copied from has one, so we could keep it.
Also I think std::string_view is fine? we are writing a string so accepting a string and converting it to bytes seems fine?
Stephen Nuskoisn't it writing data+pos into the same str that is the parameter?
Daniel AnguloObviously a char span works, but an std::string_view can be converted to a span cheaply so I'm not sure I follow the concern?
Since you already have to do base::as_writable_byte_span there is no difference but API clarity that you have a string? So better to use an std::string_view no?
base::as_writable_byte_span cannot be used with a std::string_view. Here's why
std::string_view is Read-Only: A std::string_view (and absl::string_view) provides a non-owning, read-only view into an existing character sequence. It doesn't own the data and cannot be used to modify it.
Ah right makes sense.
base::span<uint8_t> buffer(buffer_storage);Daniel AnguloYou don't need this variable, just pass `buffer_storage` directly into the SpanWriter.
And use the `subspan` method on `buffer_storage` directly for ReadOffset call.
no viable constructor or deduction guide for deduction of template arguments of 'base::SpanWriter'
589 | base::SpanWriter writer(buffer_storage);
I still prefer colapsing this to a single line:
```
base::SpanWriter writer(base::span(buffer_storage));
```
Also I think you might be able to do it with a template specification.
```
base::SpanWriter<uint8_t> writer(buffer_storage);
```
Which should remove one layer of guess work for the compiler.
Both seem more readable to me then a variable we don't use again
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
Stephen Nuskoin the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Daniel AnguloPerfect can you add a comment mentioning this? Makes sense.
Done
I'm sorry I don't see the comment?
Just something like.
```
reader.Skip(1u); // Skip the zero byte.
```
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));Daniel Angulocould this be written as a base::span::split? I think so?
Daniel AnguloDone
how? I see that there could be a split at tail_copy, but buffer_.subspan(0, copy - tail_copy) starts at offset 0 not at tail_copy, maybe I'm just a little bit confused but I don't see a clean split
I like this already better because the copy is clear what is going into what. But I think we can do the other as well.
```
auto new_buffer = base::HeapArray<uint8_t>::WithSize(size);
const size_t copy = data_length_;
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
// Split `new_buffer` into |:tail_copy:|:copy - tail_copy:| spans.
auto [new_buffer_first, new_buffer_last] =
new_buffer.as_span().first(copy).split_at(tail_copy);
// Split `buffer` into | buffer_.size() - read_position | read_position |
auto [before_read, after_read_position] =
buffer_.as_span().split_at(read_position_);
// Note: size > data_length_, but buffer_.size() >= data_length_ and unknown relation to size.
// Any value after read_position_ is the |tail_copy|, but before_read
// could be larger then the remaining |tail_copy - copy| so we adjust
// it to make them equal (old_buffer_end might be empty).
CHECK_EQ(old_buffer_start.size(), tail_copy);
old_buffer_end = old_buffer_end.first(tail_copy-copy);
new_buffer_first.copy_from_nonoverlapping(old_buffer_start);
new_buffer_last.copy_from_nonoverlapping(old_buffer_end);
```
Or something like that?
size_t tosend = std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_);
if (tosend > 0) {
base::span(block).first(tosend).copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend)));
sent = local_.Send(base::span(block).first(tosend));We do this twice? and we never use "tosend" again. Perhaps rename it into a span and use it directly?
```suggestion
auto tosend = base::span(block).first(
std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_));
if (!tosend.empty()) {
tosend.copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend.size())));
sent = local_.Send(tosend);
```
And consider this down below in other spots where it gets reused repeatedly?
int buffer_size,Daniel AnguloCan we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
check_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
Are there a lot of calls to it? Can we add the .first() to the callers of Read/Write? As Arthur said eventually we'd like to get rid of these, but if there are a lot of callers this might need to be an eventual cleanup.
It looks like each PseudoTcpAdapter::Core is only called from one spot, so could we just remove it and move the .first() into the PseudoTcpAdapter::Read/Write? although it also appears they are only called in a couple locations (like 4 files or so each), so we could move the .first() even higher there.
If you'd like to keep the CL from exploding could we at least add the CHECK arthur requested here, and update PseudoTcpAdapter::Read/Write to call .first() there? And then as a follow up you could propagate it up one level to the 4-8 files.
Related details
- Arthur Sonzogni
- Daniel Angulo
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Daniel AnguloMa?
sorry, what did you say?
Optional: Maybe this could be `“ase::span<const uint8_t>` directly. This would avoid conversion internally and avoid mixing the two types in this class.
Of course, that would move the need to convert to the callers, but I think this is better.
Related details
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 6 comments![]( "Open in Gerrit")
base::raw_span<const uint8_t> data;Do we know if there is any benchmarks around this code? This is the line most likely to trigger potential performance impact as we create and destroy segments.
found that the pseudo_tcp_unittest.cc file contains the core performance measurements, in TestTransfer method measures time of transfer. But didn't find a perftest.
bool NotifyPacket(base::span<const char> buffer);Daniel AnguloOptional: Ma
Daniel AnguloMa?
Arthur Sonzognisorry, what did you say?
Optional: Maybe this could be `“ase::span<const uint8_t>` directly. This would avoid conversion internally and avoid mixing the two types in this class.
Of course, that would move the need to convert to the callers, but I think this is better.
Done
reader.Skip(1u);Daniel AnguloWhat is this skip? isn't flags at position 13?
`ack` = `buffer + 8` which is U32 so 4 bytes so ends at `buffer + 12`, and then we do `seq.flags = buffer[13]`; but in the new code we `Skip(1u)`?
Stephen Nuskoin the writter we have buff[12] = 0; (which is the position 13) then buff[13] = flags (which is position 14). If I disable the skip then a bunch of tests start to fail
Daniel AnguloPerfect can you add a comment mentioning this? Makes sense.
Stephen NuskoDone
I'm sorry I don't see the comment?
Just something like.
```
reader.Skip(1u); // Skip the zero byte.
```
sorry my code editor reverted my comment without realizing.
const size_t tail_copy = std::min(copy, buffer_.size() - read_position_);
new_buffer.subspan(0, tail_copy)
.copy_from(buffer_.subspan(read_position_, tail_copy));
new_buffer.subspan(tail_copy, copy - tail_copy)
.copy_from(buffer_.subspan(0, copy - tail_copy));sounds good
size_t tosend = std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_);
if (tosend > 0) {
base::span(block).first(tosend).copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend)));
sent = local_.Send(base::span(block).first(tosend));We do this twice? and we never use "tosend" again. Perhaps rename it into a span and use it directly?
```suggestion
auto tosend = base::span(block).first(
std::min(static_cast<size_t>(kBlockSize),
send_buffer_.size() - send_stream_pos_));
if (!tosend.empty()) {
tosend.copy_from(base::as_bytes(
base::span(send_buffer_).subspan(send_stream_pos_, tosend.size())));
sent = local_.Send(tosend);
```And consider this down below in other spots where it gets reused repeatedly?
Done
int buffer_size,Daniel AnguloCan we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
Stephen Nuskocheck_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
Are there a lot of calls to it? Can we add the .first() to the callers of Read/Write? As Arthur said eventually we'd like to get rid of these, but if there are a lot of callers this might need to be an eventual cleanup.
It looks like each PseudoTcpAdapter::Core is only called from one spot, so could we just remove it and move the .first() into the PseudoTcpAdapter::Read/Write? although it also appears they are only called in a couple locations (like 4 files or so each), so we could move the .first() even higher there.
If you'd like to keep the CL from exploding could we at least add the CHECK arthur requested here, and update PseudoTcpAdapter::Read/Write to call .first() there? And then as a follow up you could propagate it up one level to the 4-8 files.
I'm not familiar with scoped_refptr yet, but I feel is not that simple to update PseudoTcpAdapter::Read/Write to call .first() there, the buffer parameter is a scoped_refptr<net::IOBuffer>& not a span (although it manages a span internally), it looks to be used in other places and I'm been cautious to alter the internal buffer->span() size. I was thinking on changing it to receive a span, but also see the call to reset() on the buffer which I'm not familiar of what it does but seems coupled to the original type.
Related details
- Arthur Sonzogni
- 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 added 1 comment![]( "Open in Gerrit")
base::raw_span<const uint8_t> data;Daniel AnguloDo we know if there is any benchmarks around this code? This is the line most likely to trigger potential performance impact as we create and destroy segments.
found that the pseudo_tcp_unittest.cc file contains the core performance measurements, in TestTransfer method measures time of transfer. But didn't find a perftest.
This is only used in a single location (`bool process(Segment& seg)`)
And that is only called with a reference (lifetime will always be valid)
There isn't really a point having this be a raw_ptr.
```
Pointers in local variables and function parameters and return values. This includes pointer fields in classes/structs that are used only on the stack. (Using raw_ptr<T> here would cumulatively lead to performance regression and the security benefit of UaF protection is lower for such short-lived pointers.)
```
(from [here](https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md#pointers-in-locations-other-than-fields))
So I would do the following:
```
struct Segment {
STACK_ALLOCATED();
uint32_t conv, seq, ack;
uint8_t flags;
uint16_t wnd;
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;
```
Does that make sense? This ensures it is only ever used as stack allocated (which means the pointer has to be valid if it was valid to begin with and no one calls free).
Related details
- Arthur Sonzogni
- Daniel Angulo
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
void WriteString(const char* str, size_t len) {
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.
auto bytes = base::as_byte_span(UNSAFE_BUFFERS(base::span(str, len)));There is no caller of this function? Lets just switch it to a span? or delete it.
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 4 comments![]( "Open in Gerrit")
Daniel AnguloDo we know if there is any benchmarks around this code? This is the line most likely to trigger potential performance impact as we create and destroy segments.
Stephen Nuskofound that the pseudo_tcp_unittest.cc file contains the core performance measurements, in TestTransfer method measures time of transfer. But didn't find a perftest.
This is only used in a single location (`bool process(Segment& seg)`)
And that is only called with a reference (lifetime will always be valid)
There isn't really a point having this be a raw_ptr.
```
Pointers in local variables and function parameters and return values. This includes pointer fields in classes/structs that are used only on the stack. (Using raw_ptr<T> here would cumulatively lead to performance regression and the security benefit of UaF protection is lower for such short-lived pointers.)
```(from [here](https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md#pointers-in-locations-other-than-fields))
So I would do the following:
```
struct Segment {
STACK_ALLOCATED();
uint32_t conv, seq, ack;
uint8_t flags;
uint16_t wnd;
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;
```Does that make sense? This ensures it is only ever used as stack allocated (which means the pointer has to be valid if it was valid to begin with and no one calls free).
makes sense!
void WriteString(const char* str, size_t len) {
// SAFETY: `str` is a raw char*, but we immediately convert it to a span of
// bytes, which makes our intentions clear and prevents buffer overflows.
auto bytes = base::as_byte_span(UNSAFE_BUFFERS(base::span(str, len)));Arthur SonzogniThere is no caller of this function? Lets just switch it to a span? or delete it.
(waiting on this comment to be addressed)
Done
*val = base::U32FromBigEndian(data_.subspan(pos_).first<4u>());Should we continue to use base::NetToHost32? Here and below to keep the convention?
sure
int buffer_size,Daniel AnguloCan we `CHECK_EQ(buffer->span().size(), buffer_size)`.
Eventually, the `buffer_size` argument would be removed, but this is going to take additional refactoring.
Stephen Nuskocheck_eq is throwing error because buffer->span() and buffer_size are not equal, have just added .first(buffer_size) to the Recv call
Daniel AnguloAre there a lot of calls to it? Can we add the .first() to the callers of Read/Write? As Arthur said eventually we'd like to get rid of these, but if there are a lot of callers this might need to be an eventual cleanup.
It looks like each PseudoTcpAdapter::Core is only called from one spot, so could we just remove it and move the .first() into the PseudoTcpAdapter::Read/Write? although it also appears they are only called in a couple locations (like 4 files or so each), so we could move the .first() even higher there.
If you'd like to keep the CL from exploding could we at least add the CHECK arthur requested here, and update PseudoTcpAdapter::Read/Write to call .first() there? And then as a follow up you could propagate it up one level to the 4-8 files.
I'm not familiar with scoped_refptr yet, but I feel is not that simple to update PseudoTcpAdapter::Read/Write to call .first() there, the buffer parameter is a scoped_refptr<net::IOBuffer>& not a span (although it manages a span internally), it looks to be used in other places and I'm been cautious to alter the internal buffer->span() size. I was thinking on changing it to receive a span, but also see the call to reset() on the buffer which I'm not familiar of what it does but seems coupled to the original type.
- Arthur Sonzogni
- 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 8 comments![]( "Open in Gerrit")
STACK_ALLOCATED();nit: STACK_ALLOCATED() is by convention at the top even above the `public:`
public:Segment is a struct everything is public by default, you can remove.
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}What is this for? I see the comment but I don't understand it?
1) Why would we need iterator type methods to support base::span?
2) data_ is a vector we can just return their iterators if needed .begin()/end()?
*reinterpret_cast<const uint32_t*>(data_.subspan(pos_).data()));nit: `base::NetToHost32(base::U32FromNativeEndian(data_.subspan(pos_, sizeof(uint32_t))))`
here and below
base::as_writable_byte_span(str).copy_from(data_.subspan(pos_, str.size()));This is already a span? I don't think we need a span to a span,
just use
`as_writable_bytes(str)`
this, reinterpret_cast<char*>(buffer_storage.data()), len + HEADER_SIZE);`base::as_writable_chars(buffer_storage).data()`
// Note: size > data_length_, but buffer_.size() >= data_length_ and unknownnit: `>=`
we only checked data_length_ > size which implies data_length_ <= size, and thus size is >= data_length
other->NotifyPacket(base::as_byte_span(std::string_view(packet)));Again do we want a span to the string_view? I think we just want to convert it to uint8_t?
use `base::as_bytes`
Related details
- Arthur Sonzogni
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 3 comments![]( "Open in Gerrit")
STACK_ALLOCATED();nit: STACK_ALLOCATED() is by convention at the top even above the `public:`
STACK_ALLOCATED macro has a private keyword at the begining, so if it is on placed on top all the members that follow will be private. I found moving it to the bottom a nice solution for this.
public:Segment is a struct everything is public by default, you can remove.
STACK_ALLOCATED macro is converting everything that follows to private, maybe I can remove this one, but only if it is on the bottom.
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}What is this for? I see the comment but I don't understand it?
1) Why would we need iterator type methods to support base::span?
2) data_ is a vector we can just return their iterators if needed .begin()/end()?
Compiler was complaining that this class can't be implicitly converted to a span, I had to add those methods to make it compatible with spans.
Related details
- Arthur Sonzogni
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 5 comments![]( "Open in Gerrit")
*reinterpret_cast<const uint32_t*>(data_.subspan(pos_).data()));Daniel Angulonit: `base::NetToHost32(base::U32FromNativeEndian(data_.subspan(pos_, sizeof(uint32_t))))`
here and below
I tryed that but I get: no viable conversion from 'span<element_type>' (aka 'span<const unsigned char>') to 'std::span<const uint8_t, 4U>'.
uint8_t and char are not interchangable.
base::as_writable_byte_span(str).copy_from(data_.subspan(pos_, str.size()));This is already a span? I don't think we need a span to a span,
just use
`as_writable_bytes(str)`
Done
this, reinterpret_cast<char*>(buffer_storage.data()), len + HEADER_SIZE);Daniel Angulo`base::as_writable_chars(buffer_storage).data()`
Done
// Note: size > data_length_, but buffer_.size() >= data_length_ and unknownnit: `>=`
we only checked data_length_ > size which implies data_length_ <= size, and thus size is >= data_length
Done
other->NotifyPacket(base::as_byte_span(std::string_view(packet)));Again do we want a span to the string_view? I think we just want to convert it to uint8_t?
use `base::as_bytes`
error: no matching function for call to 'as_bytes', candidate template ignored: could not match 'span' against 'basic_string_view'
Related details
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
STACK_ALLOCATED();nit: STACK_ALLOCATED() is by convention at the top even above the `public:`
STACK_ALLOCATED macro has a private keyword at the begining, so if it is on placed on top all the members that follow will be private. I found moving it to the bottom a nice solution for this.
Daniel, what about putting it at the top and then adding "public:" below to keep the members public?
```
STACK_ALLOCATED()
public:
```
Related details
- Daniel Angulo
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 2 comments![]( "Open in Gerrit")
STACK_ALLOCATED();Daniel Angulonit: STACK_ALLOCATED() is by convention at the top even above the `public:`
Arthur SonzogniSTACK_ALLOCATED macro has a private keyword at the begining, so if it is on placed on top all the members that follow will be private. I found moving it to the bottom a nice solution for this.
Daniel, what about putting it at the top and then adding "public:" below to keep the members public?
```
STACK_ALLOCATED()
public:
```
done
Daniel AnguloSegment is a struct everything is public by default, you can remove.
STACK_ALLOCATED macro is converting everything that follows to private, maybe I can remove this one, but only if it is on the bottom.
- Arthur Sonzogni
- 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 added 3 comments![]( "Open in Gerrit")
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}Daniel AnguloWhat is this for? I see the comment but I don't understand it?
1) Why would we need iterator type methods to support base::span?
2) data_ is a vector we can just return their iterators if needed .begin()/end()?
Compiler was complaining that this class can't be implicitly converted to a span, I had to add those methods to make it compatible with spans.
Yes it isn't, but this isn't the best way to make it so. Spans just need a .data() and .size() method and that is safer (no UNSAFE_TODO for example).
Can you either
1) rename `Data()` to `data() and `Length()` to `size()`
2) just add a `data()` and `size()` to make it convertable to span.
*reinterpret_cast<const uint32_t*>(data_.subspan(pos_).data()));nit: `base::NetToHost32(base::U32FromNativeEndian(data_.subspan(pos_, sizeof(uint32_t))))`
here and below
I tryed that but I get: no viable conversion from 'span<element_type>' (aka 'span<const unsigned char>') to 'std::span<const uint8_t, 4U>'.
uint8_t and char are not interchangable.
This is a fixable error though in both regards.
```
auto bytes = base::as_bytes(data_.subspan(pos_, sizeof(uint32_t))).to_fixed_extent<sizeof(uint32_t)>();`
*val = base::NetToHost32(base::u32FromNativeEndian(bytes));
```
It is always possible to get to a fixed size if you know your span is the correct size (we subspan'd it just there).
But also if you do the `base::as_bytes` to get ride of the unsigned char to uint8_t mismatch I think the compiler might have figured out to call .to_fixed_extent for you. Can you try?
other->NotifyPacket(base::as_byte_span(std::string_view(packet)));Daniel AnguloAgain do we want a span to the string_view? I think we just want to convert it to uint8_t?
use `base::as_bytes`
error: no matching function for call to 'as_bytes', candidate template ignored: could not match 'span' against 'basic_string_view'
Actually taking a step back we have a string that we want to be bytes.
Shouldn't we just go
```
base::as_byte_span(packet)
```
No need for the string_view at all?
Related details
- Arthur Sonzogni
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 3 comments![]( "Open in Gerrit")
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}Daniel AnguloWhat is this for? I see the comment but I don't understand it?
1) Why would we need iterator type methods to support base::span?
2) data_ is a vector we can just return their iterators if needed .begin()/end()?
Stephen NuskoCompiler was complaining that this class can't be implicitly converted to a span, I had to add those methods to make it compatible with spans.
Yes it isn't, but this isn't the best way to make it so. Spans just need a .data() and .size() method and that is safer (no UNSAFE_TODO for example).
Can you either
1) rename `Data()` to `data() and `Length()` to `size()`
2) just add a `data()` and `size()` to make it convertable to span.
adding data() and size() seems insufficient, I get this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'. candidate template ignored: couldn't infer template argument 'ElementType'. note: candidate template ignored: couldn't infer template argument 'ElementType'. candidate template ignored: constraints not satisfied [with R = ByteBufferWriter &] because 'ByteBufferWriter &' does not satisfy 'contiguous_range'
*reinterpret_cast<const uint32_t*>(data_.subspan(pos_).data()));Daniel Angulonit: `base::NetToHost32(base::U32FromNativeEndian(data_.subspan(pos_, sizeof(uint32_t))))`
here and below
Stephen NuskoI tryed that but I get: no viable conversion from 'span<element_type>' (aka 'span<const unsigned char>') to 'std::span<const uint8_t, 4U>'.
uint8_t and char are not interchangable.
This is a fixable error though in both regards.
```
auto bytes = base::as_bytes(data_.subspan(pos_, sizeof(uint32_t))).to_fixed_extent<sizeof(uint32_t)>();`
*val = base::NetToHost32(base::u32FromNativeEndian(bytes));
```It is always possible to get to a fixed size if you know your span is the correct size (we subspan'd it just there).
But also if you do the `base::as_bytes` to get ride of the unsigned char to uint8_t mismatch I think the compiler might have figured out to call .to_fixed_extent for you. Can you try?
Done
other->NotifyPacket(base::as_byte_span(std::string_view(packet)));Daniel AnguloAgain do we want a span to the string_view? I think we just want to convert it to uint8_t?
use `base::as_bytes`
Stephen Nuskoerror: no matching function for call to 'as_bytes', candidate template ignored: could not match 'span' against 'basic_string_view'
Actually taking a step back we have a string that we want to be bytes.
Shouldn't we just go
```
base::as_byte_span(packet)
```No need for the string_view at all?
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
Parent: 56633fc2 (Replace WIDGET_OWNS_NATIVE_WIDGET with CLIENT_OWNS_WIDGET in chrome/browser/preloading/preview)Hey Daniel!
Could you please try to compile your patches locally before requesting a code review? This would shorten the time to submit a patch, because you will be able to fix the issue before waiting on the code review.
You can also wait on the `git cl try` run to finish.
---
I fetched the patch locally and got:
```
../../remoting/protocol/pseudo_tcp.cc:121:56: error: no viable conversion from 'std::optional<span<element_type, 4UL>>' (aka 'std::optional<span<const unsigned char, 4UL>>') to 'std::span<const uint8_t, 4U>' (aka 'std::span<const unsigned char, 4U>')
121 | *val = base::NetToHost32(base::U32FromNativeEndian(bytes));
| ^~~~~
```
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Is it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
Related details
- Daniel Angulo
- 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 added 2 comments![]( "Open in Gerrit")
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Is it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
I think I just requested it as part of requesting the switch from raw_span -> span, please feel free to check if the plugin complains and you can remove it if it doesn't.
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}Daniel AnguloWhat is this for? I see the comment but I don't understand it?
1) Why would we need iterator type methods to support base::span?
2) data_ is a vector we can just return their iterators if needed .begin()/end()?
Stephen NuskoCompiler was complaining that this class can't be implicitly converted to a span, I had to add those methods to make it compatible with spans.
Daniel AnguloYes it isn't, but this isn't the best way to make it so. Spans just need a .data() and .size() method and that is safer (no UNSAFE_TODO for example).
Can you either
1) rename `Data()` to `data() and `Length()` to `size()`
2) just add a `data()` and `size()` to make it convertable to span.
adding data() and size() seems insufficient, I get this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'. candidate template ignored: couldn't infer template argument 'ElementType'. note: candidate template ignored: couldn't infer template argument 'ElementType'. candidate template ignored: constraints not satisfied [with R = ByteBufferWriter &] because 'ByteBufferWriter &' does not satisfy 'contiguous_range'
deduction guides just mean that you have to potentially be a bit more explicit about what the type is, this is because the ByteBuffer is a span of uint8_ts but doesn't tell span anything about what type it stores. However if you said `base::span<uint8_t>(buf)` it would be just fine.
In this case it is complaining about `contiguous_range` which you can read about [here](https://en.cppreference.com/w/cpp/ranges/contiguous_range.html)
but in essence it sounds like we need a `.begin()` to check the `data() == begin()`, can you
either
1) Just update the complaining callsite with the proper type.
2) Add only the .begin() iterator
3) Add full proper iterator support. I.E. add the following to the top of the class:
```
using iterator = std::vector<uint8_t>::iterator;
using const_iterator = std::vector<uint8_t>::const_iterator;
```
And then modify the iterator functions to be:
```
iterator begin() { return data_.begin(); }
const_iterator begin() const { return data_.begin(); }
iterator end() { return data_.end(); }
const_iterator end() const { return data_.end(); }
```
Then everything should work well and be properly hardened (checked iterator) without UNSAFE_TODO.
I prefer 1 or 3 over two since the iterator that returns just a pointer is unsafe if ever used to index.
Related details
- Arthur Sonzogni
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 3 comments![]( "Open in Gerrit")
Parent: 56633fc2 (Replace WIDGET_OWNS_NATIVE_WIDGET with CLIENT_OWNS_WIDGET in chrome/browser/preloading/preview)Hey Daniel!
Could you please try to compile your patches locally before requesting a code review? This would shorten the time to submit a patch, because you will be able to fix the issue before waiting on the code review.
You can also wait on the `git cl try` run to finish.
---
I fetched the patch locally and got:
```
../../remoting/protocol/pseudo_tcp.cc:121:56: error: no viable conversion from 'std::optional<span<element_type, 4UL>>' (aka 'std::optional<span<const unsigned char, 4UL>>') to 'std::span<const uint8_t, 4U>' (aka 'std::span<const unsigned char, 4U>')
121 | *val = base::NetToHost32(base::U32FromNativeEndian(bytes));
| ^~~~~```
didn't know about git cl try, thank you!
now it compiles
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Stephen NuskoIs it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
I think I just requested it as part of requesting the switch from raw_span -> span, please feel free to check if the plugin complains and you can remove it if it doesn't.
how can I check the plugin? it compiles successfully and passes the tests btw.
// Added methods for base::span support
uint8_t* begin() { return data_.data(); }
const uint8_t* begin() const { return data_.data(); }
uint8_t* end() { return UNSAFE_TODO(data_.data() + data_.size()); }
const uint8_t* end() const {
return UNSAFE_TODO(data_.data() + data_.size());
}tried 1, still complaining about contiguous range, tried 2, then is not necessary 1. No need for UNSAFE_TODO here.
Related details
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Stephen NuskoIs it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
Daniel AnguloI think I just requested it as part of requesting the switch from raw_span -> span, please feel free to check if the plugin complains and you can remove it if it doesn't.
how can I check the plugin? it compiles successfully and passes the tests btw.
The plugin is enabled by default.
Remove `RAW_PTR_EXCLUSION` (+ comment)
- If it compiles => then commit the removal
- If it doesn't => keep it.
Related details
- Arthur Sonzogni
- Daniel Angulo
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Stephen NuskoIs it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
Daniel AnguloI think I just requested it as part of requesting the switch from raw_span -> span, please feel free to check if the plugin complains and you can remove it if it doesn't.
Arthur Sonzognihow can I check the plugin? it compiles successfully and passes the tests btw.
The plugin is enabled by default.
Remove `RAW_PTR_EXCLUSION` (+ comment)
- If it compiles => then commit the removal
- If it doesn't => keep it.
- Daniel Angulo
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 Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
// RAW_PTR_EXCLUSION: Stack-scoped
RAW_PTR_EXCLUSION base::span<const uint8_t> data;Stephen NuskoIs it necessary?
I know the clang plugin does exclude `STACK_ALLOCATED()`:
https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/raw_ptr_plugin/RawPtrHelpers.h;l=495-504?q=should_exclude_stack_allocated_records%20-f:third_party
Daniel AnguloI think I just requested it as part of requesting the switch from raw_span -> span, please feel free to check if the plugin complains and you can remove it if it doesn't.
Arthur Sonzognihow can I check the plugin? it compiles successfully and passes the tests btw.
Arthur SonzogniThe plugin is enabled by default.
Remove `RAW_PTR_EXCLUSION` (+ comment)
- If it compiles => then commit the removal
- If it doesn't => keep it.
*(replying with my @google account to wait on this)*
it does compiles with the removal, so i'm commiting the removal. thanks!
Related details
- Arthur Sonzogni
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 7 comments![]( "Open in Gerrit")
Thanks!
auto bytes = base::byte_span_from_ref(nval);
data_.insert(data_.end(), bytes.begin(), bytes.end());
}Would that work:
```
data_.append_range(bytes);
```
?
uint16_t nval = base::HostToNet16(val);
auto bytes = base::byte_span_from_ref(nval);
data_.insert(data_.end(), bytes.begin(), bytes.end());
}ditto? `std::vector::append_range`
auto buffer_storage = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::span<uint8_t> buffer(buffer_storage);
base::SpanWriter writer(buffer);I suspect HeapArray is convertible to base::span implicitly:
```
auto buffer = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::SpanWriter writer(buffer);
```
It it doesn't, you can do it explictely:
```
```
auto buffer = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::SpanWriter writer(buffer.as_span());
```
queue(base::span(buf), true);I suspect you don't need to explicitly use `base::span`, because it has an implicit constructor:
```
queue(buf, true)
```
Does that work?
new_buffer_first.copy_from_nonoverlapping(old_buffer_start);Let's always `copy_from`. This is always safe, and we don't have evidence the `copy_from_nonoverlapping` to be slower.
received = receiver_->Recv(base::span(block));ditto: (remove base::span)
Related details
- Arthur Sonzogni
- Daniel Angulo
- 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 added 4 comments![]( "Open in Gerrit")
#include "base/memory/raw_span.h"This is now unused right?
(actually appears to be in the .cc so should be moved there in the current patchset, but I added a comment to remove that usage as well so probably delete it along with doing that comment).
base::raw_span<const uint8_t> data_;Question can you make this class be STACK_ALLOCATED as well? Appears to be used as such and this can be a regular span.
Or...
Just remove this class entirely? It is just a SpanReader no? Doesn't need its own class?
Add a similar `CHECK_EQ(buffer->span().size(), buffer_size);`?
int result = pseudo_tcp_.Send(buffer->first(buffer_size));nit: isn't this `buffer->span()`? here and below? or is there a difference here for some reason?
Related details
- Arthur Sonzogni
- Daniel Angulo
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 1 comment![]( "Open in Gerrit")
*val = base::NetToHost32(base::U32FromNativeEndian(bytes.value()));Is this correct? Is there good tests for this? wondering because in the writer it does `HostToNet16` (so the wire format is always big endian), so perhaps this should be:
`base::NetToHost32(base::U32FromBigEndian(bytes.value()));`?
Can you verify the old behaviour and ensure we're properly replicating the behaviour? Perhaps check that there is a test covering this case?
Daniel Angulo (Gerrit)
Daniel Angulo added 9 comments![]( "Open in Gerrit")
This is now unused right?
(actually appears to be in the .cc so should be moved there in the current patchset, but I added a comment to remove that usage as well so probably delete it along with doing that comment).
Done
auto bytes = base::byte_span_from_ref(nval);
data_.insert(data_.end(), bytes.begin(), bytes.end());
}Would that work:
```
data_.append_range(bytes);
```
?
Done
uint16_t nval = base::HostToNet16(val);
auto bytes = base::byte_span_from_ref(nval);
data_.insert(data_.end(), bytes.begin(), bytes.end());
}Daniel Anguloditto? `std::vector::append_range`
Done
auto buffer_storage = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::span<uint8_t> buffer(buffer_storage);
base::SpanWriter writer(buffer);I suspect HeapArray is convertible to base::span implicitly:
```
auto buffer = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::SpanWriter writer(buffer);
```It it doesn't, you can do it explictely:
```
```
auto buffer = base::HeapArray<uint8_t>::WithSize(MAX_PACKET);
base::SpanWriter writer(buffer.as_span());
```
Done
I suspect you don't need to explicitly use `base::span`, because it has an implicit constructor:
```
queue(buf, true)
```Does that work?
Done
new_buffer_first.copy_from_nonoverlapping(old_buffer_start);Let's always `copy_from`. This is always safe, and we don't have evidence the `copy_from_nonoverlapping` to be slower.
Done
received = receiver_->Recv(base::span(block));Daniel Anguloditto: (remove base::span)
Done
Add a similar `CHECK_EQ(buffer->span().size(), buffer_size);`?
check fails with: buffer->span().size() == buffer_size (102400 vs. 1024). and because of that it needs "first" in line 189
int result = pseudo_tcp_.Send(buffer->first(buffer_size));nit: isn't this `buffer->span()`? here and below? or is there a difference here for some reason?
check in line 185, fails with: buffer->span().size() == buffer_size (102400 vs. 1024). and because of that it needs "first" in line 189. there was a similar thread about this in PseudoTcpAdapter::Core::Read line 154
Related details
- Arthur Sonzogni
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
class ByteBufferReader {
public:
explicit ByteBufferReader(base::span<const uint8_t> data) : data_(data) {}
bool ReadUInt32(uint32_t* val) {
auto bytes = base::as_bytes(data_.subspan(pos_, sizeof(uint32_t)))
.to_fixed_extent<sizeof(uint32_t)>();
*val = base::NetToHost32(base::U32FromBigEndian(bytes.value()));
pos_ += sizeof(uint32_t);
return true;
}
bool ReadUInt16(uint16_t* val) {
if (pos_ + sizeof(uint16_t) > data_.size()) {
return false;
}
*val = base::NetToHost16(
*reinterpret_cast<const uint16_t*>(data_.subspan(pos_).data()));
pos_ += sizeof(uint16_t);
return true;
}
bool ReadUInt8(uint8_t* val) {
if (pos_ + sizeof(uint8_t) > data_.size()) {
return false;
}
*val = data_[pos_++];
return true;
}
bool ReadString(base::span<char> str) {
if (pos_ + str.size() > data_.size()) {
return false;
}
base::as_writable_bytes(str).copy_from(data_.subspan(pos_, str.size()));
pos_ += str.size();
return true;
}
size_t Length() const { return data_.size() - pos_; }
const uint8_t* Data() const { return &data_[pos_]; }
void Consume(size_t len) { pos_ += len; }
STACK_ALLOCATED();
base::span<const uint8_t> data_;
size_t pos_ = 0;
};`STACK_ALLOCATED()` should be added at the top of the class.
Alos, I wonder if we couldn't remove ByteBufferReader in favor of base::SpanReader?
Related details
- Arthur Sonzogni
- Daniel Angulo
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. |