refactor: pass references to objects instead of using global definitions - #4988
Merged
Conversation
|
This pull request has conflicts, please rebase. |
kwvg
force-pushed
the
prectx
branch
10 times, most recently
from
September 3, 2022 19:12
50d6627 to
59b1fe0
Compare
UdjinM6
previously requested changes
Sep 6, 2022
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
pls see 0baef90b86 + 0968c71533 (to avoid dereferencing it too early) OR maybe 5c48866762 instead (to avoid dereferencing completely)
Co-authored-by: "UdjinM6 <UdjinM6@users.noreply.github.com>"
PastaPastaPasta
approved these changes
Sep 20, 2022
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
Member
There was a problem hiding this comment.
utACK for squash merge
michaelvkax
pushed a commit
to vkaxproject/vkax
that referenced
this pull request
Oct 22, 2022
…ons (dashpay#4988) * fix: move chain activation logic downward to succeed LLMQ initialization * fix: change order of initialization to reflect dependency * llmq: pass all global pointers invoked as CDSNotificationInterface arguments * llmq: pass reference to quorumDKGDebugManager instead of invoking global * llmq: pass reference to quorumBlockProcessor instead of invoking global * llmq: pass reference to quorumDKGSessionManager instead of invoking global * llmq: pass reference to quorumManager instead of invoking global Co-authored-by: "UdjinM6 <UdjinM6@users.noreply.github.com>" * llmq: pass reference to quorumSigSharesManager within CSigningManager and networking * llmq: pass reference to quorumSigSharesManager instead of invoking global * llmq: pass reference to chainLocksHandler instead of querying global * llmq: pass reference to quorumInstantSendManager instead of querying global * trivial: accept argument as const where possible * style: remove an unneeded const_cast and instead pass by const reference * style: use const where possible Co-authored-by: pasta <pasta@dashboost.org>
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 4, 2026
…syncSignIfMember 7c05e56 refactor: drop redundant CSigningManager argument from AsyncSignIfMember (pasta) Pull request description: ## Issue being fixed or feature implemented `CSigSharesManager::AsyncSignIfMember` takes a `CSigningManager&` argument it does not need. The class has held a `CSigningManager& sigman` member since a352456 (#4988), declared 34 lines above the method in the same header, and the parameter **shadows** it. There is exactly one `CSigningManager` in a node — `node.llmq_ctx->sigman`. `init.cpp` threads it into `ActiveContext`, which hands the same reference to `CSigSharesManager`'s member and to each caller's own member, so every call site already passes the object the member points at: | Call site | Passes | |---|---| | `chainlock/signing.cpp:177` | `m_sigman` | | `instantsend/signing.cpp:356`, `:415` | `m_sigman` | | `llmq/ehf_signals.cpp:82` | `sigman` | | `rpc/quorums.cpp:533` | `*llmq_ctx.sigman` | The parameter comes from 0052fca ("refactor: move `AsyncSignIfMember()` to `CSigSharesManager`"). Before that move the method lived on `CSigningManager` and took the *opposite* manager, `CSigSharesManager& shareman`. After the move that argument became `this`, and a `sigman` argument was added to reach back for the recovered-sigs db — a mirror-image swap that is correct in isolation, but that nobody reconciled with the member the destination class already had. Consequence today: the single `sigman.GetDb()` in the body resolves to the argument, while the surrounding code in the same function reaches for `qman` and `m_chainman` as members. ## What was done? Dropped the `CSigningManager& sigman` parameter from the declaration, the definition, and all five call sites. The body is untouched — `sigman` now resolves to the member, which is the same object it already received. Parameter-list continuation lines keep their alignment because the opening paren does not move. ## How Has This Been Tested? Not built or run locally — reviewed by inspection. Verified by grep that no declaration, definition, or call site still passes a `CSigningManager` to `AsyncSignIfMember`, and that removing the argument leaves no caller-side variable unused (`llmq_ctx` in `rpc/quorums.cpp` is still used a few lines below). Relying on CI for the compile and test run. Worth noting for reviewers: this class of shadowing is invisible to our warning set. Dash configures `-Wshadow-field`, which only fires when a field shadows a *base class* field; a parameter shadowing a member of the same class needs `-Wshadow-all` (or GCC's `-Wshadow`). ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 64cb21e35730b4008804d719664810c851c4a8c84f7f2474d3c9104b75ccb00985fb8607a3cda83797e8b3d0bebfb94d278f54d2ae73faa3b2002bb790cfbcc6
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
No description provided.