refactor: memcpy -> std::ranges::copy - #307
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
This avoids a std::span ctor. Recommended in bitcoin-core#305 (review)
Recommended by the LLM in bitcoin/bitcoin#35684 (comment)
ViniciusCestarii
left a comment
ViniciusCestarii
left a comment
There was a problem hiding this comment.
ACK 8226c05 just commented a nit
| #include <mp/util.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <ranges> |
There was a problem hiding this comment.
In "refactor: memcpy -> std::ranges::copy" 8226c05
nit: I believe this #include <ranges> is not necessary here
There was a problem hiding this comment.
Isn't the iwyu CI supposed to catch this? It passes with either version ...
Maybe it could make sense to fix the CI instead, so that all places are fixed and not only the ones caught in review?
There was a problem hiding this comment.
The CI runs IWYU and it only audits "the input .cc file and its associated .h files" (e.g. foo.cpp also checks foo.h). type-string.h has no matching .cpp so it isn't audited.
- set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXECUTABLE};-Xiwyu;--error")
+ set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXECUTABLE};-Xiwyu;--error;-Xiwyu;--check_also=${PROJECT_SOURCE_DIR}/include/mp/*.h")With the above diff it checks the headers too and can check for type-string.h. (Bitcoin core also uses --check_also for primitive headers):
/home/vinicius/Code/my/libmultiprocess/include/mp/type-string.h should add these lines:
#include <string> // for string
namespace mp { struct InvokeContext; }
/home/vinicius/Code/my/libmultiprocess/include/mp/type-string.h should remove these lines:
- #include <ranges> // lines 11-11
It also flag a lot of what there is currently (10+ header files) and can also make false positives: note the #include <variant> // for tuple which doens't make sense for proxy-io.h:
/home/vinicius/Code/my/libmultiprocess/include/mp/proxy-io.h should add these lines:
#include <capnp/capability.h> // for Capability, CallContext, Capab...
#include <capnp/common.h> // for Void, word
#include <capnp/message.h> // for MallocMessageBuilder, ReaderOp...
#include <capnp/rpc-twoparty.capnp.h> // for VatId, Side, Side_9fd69ebc87b9...
#include <capnp/rpc.h> // for RpcSystem, makeRpcClient, make...
#include <kj/async-io.h> // for LowLevelAsyncIoProvider, Async...
#include <kj/async-prelude.h> // for ReadyNow
#include <kj/async.h> // for TaskSet, Promise, READY_NOW
#include <kj/common.h> // for mv, ArrayPtr, KJ_IF_MAYBE, Maybe
#include <kj/exception.h> // for runCatchingExceptions, Exception
#include <kj/memory.h> // for Own, heap
#include <kj/string.h> // for KJ_STRINGIFY, StringPtr
#include <list> // for _List_iterator, list, _List_co...
#include <tuple> // for tuple
#include <utility> // for forward, move
#include <variant> // for tuple
#include <vector> // for vector
namespace mp { class Connection; }
namespace mp { class EventLoop; }
namespace mp { template <typename Interface> struct ProxyClient; }
namespace mp { template <typename Interface> struct ProxyServer; }
So I believe this isn't a one line fix, it would require a one-time PR to manually audit and fix and map the false positives in a mapping file and then enabling --check_also and --mapping_file with the mapping file to handle false positives on the CI. Maybe this is not worth effort.
This is just what I found poking around, if there's a better way id like to hear it.
There was a problem hiding this comment.
re: #307 (comment)
Wow the horrors of IWYU are unending! Would review patch, but this adds to my regrets from bitcoin/bitcoin#10575 (comment)
There was a problem hiding this comment.
thx, removed #include <ranges> for now
| auto data = std::span{value}; | ||
| auto result = output.init(data.size()); | ||
| std::ranges::copy(data, result.begin()); | ||
| auto result = output.init(value.size()); |
There was a problem hiding this comment.
In commit "refactor: Directly use value in CustomBuildField" (ce865a9)
This looks like a it's a good change, but it's not really implementing the suggestion to drop the result variable. It's doing something different and dropping the data variable (which is good and I didn't know was possible). Maybe consider updating the commit message or dropping result too
There was a problem hiding this comment.
This is mostly for consistency with string include/mp/type-string.h, which also has the allocation in a separate line:
auto result = output.init(value.size());| return read_dest.update([&](auto& value) { | ||
| auto data = input.get(); | ||
| memcpy(value, data.begin(), size); | ||
| std::ranges::copy(input.get(), std::ranges::begin(value)); |
There was a problem hiding this comment.
In commit "refactor: memcpy -> std::ranges::copy" (8226c05)
This is ok, but it is now deciding how much data to copy based on the length of the input instead of the length of the output, so previously if there was a mismatch the code could read too many bytes, and now it could write too many bytes.
More ideally this would ensure the size matches with something like:
auto data = input.get();
if (data.size() != size) throw std::range_error("unexpected field size");| #include <mp/util.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <ranges> |
There was a problem hiding this comment.
re: #307 (comment)
Wow the horrors of IWYU are unending! Would review patch, but this adds to my regrets from bitcoin/bitcoin#10575 (comment)
I think there is no issue of passing a nullptr to memcpy here, but it makes sense to use the std-lib copy for consistency. Recommended in bitcoin-core#305 (review) Also, add a size sanity check when reading a char array.
4 participants
Some follow-ups to #305 (review)