refactor: Pass lifetimebound reference to SingleThreadedSchedulerClient #25040
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14 tasks
|
ACK fa4652c Built and debugged the changes locally. |
jonatack
reviewed
May 3, 2022
There was a problem hiding this comment.
Code review ACK fa4652c rebased to master, debug build, unit tests
This commit:
- passes the
SingleThreadedSchedulerClient::schedulerparam by reference instead of a pointer, which allows dropping a runtime non-nullptr assertion inSingleThreadedSchedulerClient::AddToProcessQueue()to avoid nullptr dereferencing (similar change in recently-merged #25016) - adds the clang
LIFETIMEBOUNDattribute to theSingleThreadedSchedulerClientandMainSignalsInstanceconstructors to avoid call sites passing a temporary - renames
m_pschedulertom_scheduler - adds missing header includes to
scheduler.h - replaces a less-efficient
std::bindwith a lambda - replaces a
reset(new)with preferredstd::make_unique
clang::lifetimebound is described in https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound:
The lifetimebound attribute indicates that a resource owned by a function parameter or implicit object parameter is retained by the return value of the annotated function (or, for a parameter of a constructor, in the value of the constructed object).
maflcko
pushed a commit
that referenced
this pull request
May 4, 2022
…)::start_time 4cb9d21 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack) Pull request description: Suggested in #25016 (comment), the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time. See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and #22278 for related discussion, and #25040 for a similar example. ACKs for top commit: MarcoFalke: review ACK 4cb9d21 Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 4, 2022
…ThreadedSchedulerClient fa4652c Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake) Pull request description: Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference. All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines. ACKs for top commit: pk-b2: ACK bitcoin@fa4652c jonatack: Code review ACK fa4652c rebased to master, debug build, unit tests vincenzopalazzo: ACK bitcoin@fa4652c Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 4, 2022
…dBlock()::start_time 4cb9d21 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack) Pull request description: Suggested in bitcoin#25016 (comment), the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time. See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and bitcoin#22278 for related discussion, and bitcoin#25040 for a similar example. ACKs for top commit: MarcoFalke: review ACK 4cb9d21 Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 15, 2023
Summary: > Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference. > > All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines. This is a backport of [[bitcoin/bitcoin#25040 | core#25040]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13127
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference.
All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines.