Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
maflcko wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce865a9
refactor: Directly use value in CustomBuildField
Jul 14, 2026
17eab90
test: Fix typo in listen_tests.cpp
Jul 14, 2026
8226c05
refactor: memcpy -> std::ranges::copy
Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions include/mp/type-char.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

#include <mp/util.h>

#include <algorithm>
#include <ranges>

namespace mp {
template <typename Output, size_t size>
void CustomBuildField(TypeList<const unsigned char*>,
Expand All @@ -16,7 +19,7 @@ void CustomBuildField(TypeList<const unsigned char*>,
Output&& output)
{
auto result = output.init(size);
memcpy(result.begin(), value, size);
std::ranges::copy(value, result.begin());
}

template <size_t size, typename Input, typename ReadDest>
Expand All @@ -27,8 +30,7 @@ decltype(auto) CustomReadField(TypeList<unsigned char[size]>,
ReadDest&& read_dest)
{
return read_dest.update([&](auto& value) {
auto data = input.get();
memcpy(value, data.begin(), size);
std::ranges::copy(input.get(), std::ranges::begin(value));
});
}
} // namespace mp
Expand Down
5 changes: 2 additions & 3 deletions include/mp/type-data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@ template <typename LocalType, typename Value, typename Output>
void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Value&& value, Output&& output)
requires (std::is_same_v<decltype(output.get()), ::capnp::Data::Builder> && IsByteSpan<LocalType>)
{
auto data = std::span{value};
auto result = output.init(data.size());
std::ranges::copy(data, result.begin());
auto result = output.init(value.size());
std::ranges::copy(value, result.begin());
}

template <typename LocalType, typename Input, typename ReadDest>
Expand Down
5 changes: 4 additions & 1 deletion include/mp/type-string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

#include <mp/util.h>

#include <algorithm>
#include <ranges>

@ViniciusCestarii ViniciusCestarii Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@maflcko maflcko Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ViniciusCestarii Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


namespace mp {
template <typename Value, typename Output>
void CustomBuildField(TypeList<std::string>,
Expand All @@ -16,7 +19,7 @@ void CustomBuildField(TypeList<std::string>,
Output&& output)
{
auto result = output.init(value.size());
memcpy(result.begin(), value.data(), value.size());
std::ranges::copy(value, result.begin());
}

template <typename Input, typename ReadDest>
Expand Down
2 changes: 1 addition & 1 deletion test/mp/test/listen_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ KJ_TEST("ListenConnections enforces a local connection limit")
{
// With max-connections=1, the second socket can connect to the kernel
// backlog, but ListenConnections should not accept or serve it until the
// first accepts clients disconnects.
// first accepted client disconnects.

ListenSetup server(/*max_connections=*/1);

Expand Down
Loading