Skip to content

Conversation

ryanofsky
Copy link
Collaborator

There should be a lot of places where new C++ features can be used to simplify code, particularly concept features in C++20 so I'm starting to go through code and look for places where it can be modernized. Will collect the smaller cleanups in this PR and probably open other PRs with bigger cleanups.

ryanofsky added a commit that referenced this pull request Jun 13, 2024
537c645 doc: Improve ProxyServerCustom class documentation (Ryan Ofsky)
d4d9f93 doc: Document FunctionTraits/ProxyMethodTraits classes (Ryan Ofsky)
78c7dd0 doc: Document ProxyClient construct/destroy methods (Ryan Ofsky)
e99c0b7 doc: Document clientInvoke/serverInvoke functions (Ryan Ofsky)
2098ae1 doc: Add comment on serverInvoke ReplaceVoid usage (Ryan Ofsky)
a1dfb0b doc: Add comments to mp.Context PassField function on updating g_thread_context (Ryan Ofsky)
e49a925 doc: Add comments to mp.Context PassField function on mp.Context.thread lookup (Ryan Ofsky)

Pull request description:

  Add code comments and documentation. Most of these comments were originally added as part of #94 but are being moved to a separate PR so they can be merged sooner

Top commit has no ACKs.

Tree-SHA512: 284325513d3244313a4ff54ed20648061a6142ce89f4449bb046938780f1d74c7691dba5fdff5f478f585e95e5ea0987af2fde2e67c049adc8a6b7db21371341
Also add comments to explain code using it the PassField proxy.capnp Context
overload
@fanquake
Copy link
Member

Maybe also time to revive these changes?

@ryanofsky
Copy link
Collaborator Author

Added 1 commits 1f76880 -> 6cbe56e (pr/cl1.4 -> pr/cl1.5, compare) adding a new commit to make lambda capture order a little more consistent.

There is more work that could be done here, but I'm about to merge this PR to clear backlog of libmultiprocess changes before bumping the version in bitcoin.

re: #94 (comment)

Maybe also time to revive these changes?

Yes, there is definitely more work that could be done to modernize the code, which was originally written using c++11. Should be able to happen in separate followups though.

@ryanofsky ryanofsky merged commit 10bb7e4 into bitcoin-core:master Jan 27, 2025
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 27, 2025
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 27, 2025
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Jan 28, 2025
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Jan 29, 2025
…ng to subtree

4e0aa18 test: Add test for IPC serialization bug (Ryan Ofsky)
2221c88 depends: Update libmultiprocess library before converting to subtree (Ryan Ofsky)

Pull request description:

  This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, followup PR #31741 will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash.

  Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them.

  This update has the following new changes since previous update #31105:

  bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
  bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
  bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
  bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
  bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
  bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
  bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
  bitcoin-core/libmultiprocess#94 c++ 20 cleanups
  bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
  bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
  bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links

ACKs for top commit:
  Sjors:
    ACK 4e0aa18
  vasild:
    ACK 4e0aa18
  TheCharlatan:
    ACK 4e0aa18

Tree-SHA512: 6d81cdf7f44762c7f476212295f6224054fd0a61315bb54786bc7758a2b33e5a2fce925c71e36f7bda320049aa14e7218a458ceb03dacbb869632c466c4789b0
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Sep 1, 2025
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants