TEST/PROTO: Reset request tests for offset = 0 #9553
Conversation
shasson5
changed the title
shasson5
force-pushed
the
prot_res_test
branch
from
7991dca to
20f3869
Compare
test/gtest/ucp/test_ucp_request.cc
Outdated
|
|
||
| static void flushed_cb(ucp_request_t *request) | ||
| { | ||
| test_proto_reset *self = (test_proto_reset*)request->user_data; |
There was a problem hiding this comment.
minor: C++ static_cast instead of C-cast would be more idiomatic here
test/gtest/ucp/test_ucp_request.cc
Outdated
| ucp_request_release(request); | ||
| } | ||
|
|
||
| void send_nb(std::vector<char> &sbuf, std::vector<char> *rbuf_p, |
There was a problem hiding this comment.
why not to pass rbuf by reference?
There was a problem hiding this comment.
code changed (using mapped_buffer now)
test/gtest/ucp/test_ucp_request.cc
Outdated
| ucp_request_param_t param = {0}; | ||
| void *rreq = NULL; | ||
| void *sreq = NULL; | ||
| std::vector<char> &rbuf = *rbuf_p; |
There was a problem hiding this comment.
This variable seems to be redundant
test/gtest/ucp/test_ucp_request.cc
Outdated
| return UCS_OK; | ||
| } | ||
|
|
||
| void *send_am(std::vector<char> &sbuf, std::vector<char> *rbuf_p) |
There was a problem hiding this comment.
better pass rbuf by reference here?
test/gtest/ucp/test_ucp_request.cc
Outdated
| { | ||
| std::vector<char> *rbuf = (std::vector<char>*)arg; | ||
|
|
||
| memcpy((*rbuf).data(), data, length); |
There was a problem hiding this comment.
I'm afraid this might not be always safe, what if vector size is smaller than length?
better alternative is to use std::vector::assign(data, data + length)
There was a problem hiding this comment.
rbuf was changed to be a mapped_buffer object so instead I added an assertion on the size
test/gtest/ucp/test_ucp_request.cc
Outdated
| continue; | ||
| } | ||
|
|
||
| memcpy(&rbuf[roffset], rdata, length); |
There was a problem hiding this comment.
In general it's not safe to append elements to vector like that, it disrespects vector invariants.
Why not using std::vector::insert that accepts a range?
There was a problem hiding this comment.
I use a different API now (ucp_stream_recv_nbx) so memcpy is not needed at all
test/gtest/ucp/test_ucp_request.cc
Outdated
| void *rreq = NULL; | ||
| void *sreq = NULL; | ||
| std::vector<char> &rbuf = *rbuf_p; | ||
| sbuf.resize(m_msg_size); |
There was a problem hiding this comment.
maybe here we can just reserve, not resize?
There was a problem hiding this comment.
code was changed (now init the array inside a for loop).
shasson5
force-pushed
the
prot_res_test
branch
from
3581fd8 to
bca427b
Compare
What
Reset request tests for offset = 0
Why ?
First phase for reset in the middle PR.