refactor: memcpy -> std::ranges::copy #307

maflcko commented at 9:11 AM on July 14, 2026: contributor

Some follow-ups to #305#pullrequestreview-4689778284

DrahtBot commented at 9:11 AM on July 14, 2026: none

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ryanofsky
Stale ACK	ViniciusCestarii

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

refactor: Directly use value in CustomBuildField

This avoids a std::span ctor.

Recommended in https://github.com/bitcoin-core/libmultiprocess/pull/305#pullrequestreview-4689778284

ce865a9ba8

test: Fix typo in listen_tests.cpp

Recommended by the LLM in https://github.com/bitcoin/bitcoin/pull/35684#issuecomment-4916571942

17eab90b52

maflcko force-pushed on Jul 17, 2026

in include/mp/type-string.h:11 in 8226c05549

   6 | @@ -7,6 +7,9 @@
   7 |  
   8 |  #include <mp/util.h>
   9 |  
  10 | +#include <algorithm>
  11 | +#include <ranges>

ViniciusCestarii commented at 3:10 PM on July 17, 2026:

In "refactor: memcpy -> std::ranges::copy" 8226c05549b0405ebac3208751c53a03563ea745

nit: I believe this #include <ranges> is not necessary here

maflcko commented at 3:44 PM on July 17, 2026:

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?

ViniciusCestarii commented at 7:34 PM on July 17, 2026:

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.

ryanofsky commented at 3:54 AM on July 30, 2026:

re: #307 (review)

Wow the horrors of IWYU are unending! Would review patch, but this adds to my regrets from https://github.com/bitcoin/bitcoin/pull/10575#issuecomment-307782757

maflcko commented at 11:52 AM on July 31, 2026:

thx, removed #include <ranges> for now

ViniciusCestarii commented at 3:20 PM on July 17, 2026: contributor

ACK 8226c05549b0405ebac3208751c53a03563ea745 just commented a nit

in include/mp/type-data.h:35 in ce865a9ba8 outdated

  31 | @@ -32,9 +32,8 @@ template <typename LocalType, typename Value, typename Output>
  32 |  void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Value&& value, Output&& output)
  33 |  requires (std::is_same_v<decltype(output.get()), ::capnp::Data::Builder> && IsByteSpan<LocalType>)
  34 |  {
  35 | -    auto data = std::span{value};
  36 | -    auto result = output.init(data.size());
  37 | -    std::ranges::copy(data, result.begin());
  38 | +    auto result = output.init(value.size());

ryanofsky commented at 3:24 AM on July 30, 2026:

In commit "refactor: Directly use value in CustomBuildField" (ce865a9ba86e603e6eb69aba53f791203b80fcb2)

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

maflcko commented at 11:52 AM on July 31, 2026:

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());

in include/mp/type-char.h:33 in 8226c05549 outdated

  29 | @@ -27,8 +30,7 @@ decltype(auto) CustomReadField(TypeList<unsigned char[size]>,
  30 |      ReadDest&& read_dest)
  31 |  {
  32 |      return read_dest.update([&](auto& value) {
  33 | -        auto data = input.get();
  34 | -        memcpy(value, data.begin(), size);
  35 | +        std::ranges::copy(input.get(), std::ranges::begin(value));

ryanofsky commented at 3:47 AM on July 30, 2026:

In commit "refactor: memcpy -> std::ranges::copy" (8226c05549b0405ebac3208751c53a03563ea745)

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");

maflcko commented at 11:52 AM on July 31, 2026:

thx, done

ryanofsky approved

ryanofsky commented at 3:57 AM on July 30, 2026: collaborator

Code review ACK 8226c05549b0405ebac3208751c53a03563ea745. Thanks for the followup. Left some suggestions but also would be happy if the PR were merged as-is

refactor: memcpy -> std::ranges::copy

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 https://github.com/bitcoin-core/libmultiprocess/pull/305#pullrequestreview-4689778284

Also, add a size sanity check when reading a char array.

26452e02d7

maflcko force-pushed on Jul 31, 2026

ryanofsky approved

ryanofsky commented at 1:59 PM on August 3, 2026: collaborator

Code review ACK 26452e02d75e8a2a33752ef181f7ffc96ca968f0. Thanks for the updates!

DrahtBot requested review from ViniciusCestarii on Aug 3, 2026

ryanofsky merged this on Aug 3, 2026

ryanofsky closed this on Aug 3, 2026

maflcko deleted the branch on Aug 4, 2026