refactor: reduce ostringstream usage, set UniValue variable to object during construction, reduce GetHash() usage
#6635
Conversation
|
This pull request has conflicts, please rebase. |
WalkthroughThis set of changes primarily focuses on code simplification, modernization, and minor test and utility enhancements across several source files. Many updates replace multi-step or manual constructions of objects and strings with more concise and idiomatic alternatives. Specifically, numerous instances of ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
src/llmq/commitment.h (1)
124-124: Code simplification: direct UniValue object initialization.Replaced two-step initialization with a more concise direct constructor call. This makes the code more readable while maintaining identical functionality.
This change is part of a consistent pattern across multiple files to standardize UniValue initialization.
src/netfulfilledman.cpp (1)
90-90: Improved string formatting using strprintf.Replaced ostringstream usage with the more concise strprintf function, which improves readability and is more consistent with the codebase's string formatting practices.
This change aligns with the PR's goal of reducing ostringstream usage throughout the codebase in favor of more direct string formatting.
src/governance/exceptions.cpp (1)
36-38: Improved constructor implementation with modern C++ initializationGood refactoring to use uniform initialization and
strprintfinstead of stream-based string construction. This makes the code more maintainable and potentially more efficient.Note that the pipeline indicates a clang format issue on line 30 (unrelated to your changes). Consider running clang-format on the file to fix this issue.
src/governance/object.cpp (1)
69-71: Improved string formatting with strprintf and funcGood refactoring to replace manual string stream construction with
strprintffor log and error messages. Using__func__to dynamically include the function name is a better practice than hardcoding function names, as it ensures messages stay accurate if function names change.The replacement of
std::ostringstreamwithstrprintflikely improves both readability and performance.The pipeline indicates a formatting issue in lines 75-156. Consider running clang-format on the file to ensure consistent styling.
Also applies to: 77-79, 87-89, 93-96, 104-106, 111-121, 129-133, 143-147, 153-157
src/governance/governance.cpp (1)
1134-1144: Improved string formatting with strprintf and funcGood refactoring to use
strprintfwith__func__for log messages instead of hardcoding function names. This approach ensures that log messages stay accurate even if function names change, and generally leads to more readable and maintainable code.The pipeline indicates formatting issues in lines 1132-1168. Consider running clang-format on the file to address these style inconsistencies.
src/rpc/masternode.cpp (8)
265-269: Use idiomatic emptiness check and consider caching
You've replacedstrFilter != ""with!strFilter.empty()in both loop branches, which is clearer and more efficient. As an optional refactor, you could hoistbool hasFilter = !strFilter.empty()before the loops to avoid repeated method calls.Also applies to: 271-277
569-571: Apply.empty()check in 'addr' mode
Switching to!strFilter.empty()for the address filter is more idiomatic. For performance, you might cache this boolean before entering the loop.
627-630: Use.empty()in 'lastpaidblock' filter
Switching to!strFilter.empty()is more idiomatic. As a possible enhancement, consider filtering on the block height string itself if that’s a user use-case.
631-633: Apply.empty()check in 'lastpaidtime' mode
Good adoption of!strFilter.empty(). You may cache the result outside the loop to shave off a few function calls.
634-637: Refactor filter in 'payee' mode
Using.empty()here is spot-on. Optionally, you could also filter on thepayeeStrand outpoint uniformly by cachinghasFilter.
638-640: Enhance filter in 'owneraddress' mode
Currently only the outpoint is filtered. It may be more intuitive to also matchstrFilteragainst the owner address string returned.
641-643: Improve filter in 'pubkeyoperator' mode
For consistency, consider adding a check against the operator public key string in addition to the outpoint.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 567-643: Clang format differences found. Code formatting does not match the expected style.
649-651: Extend filter in 'votingaddress' mode
You currently filter only on the outpoint. For parity with other modes, consider matchingstrFilteragainst the voting address as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/evo/deterministicmns.cpp(1 hunks)src/evo/dmnstate.cpp(2 hunks)src/evo/mnhftx.h(1 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/governance/classes.cpp(3 hunks)src/governance/exceptions.cpp(2 hunks)src/governance/governance.cpp(1 hunks)src/governance/object.cpp(3 hunks)src/governance/vote.cpp(1 hunks)src/llmq/commitment.h(1 hunks)src/llmq/snapshot.cpp(2 hunks)src/masternode/meta.cpp(1 hunks)src/netfulfilledman.cpp(1 hunks)src/rpc/evo.cpp(4 hunks)src/rpc/masternode.cpp(4 hunks)src/test/util_tests.cpp(1 hunks)src/txmempool.cpp(8 hunks)src/util/string.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/util_tests.cpp (1)
src/util/string.h (1)
PadString(60-60)
src/txmempool.cpp (2)
src/txmempool.h (21)
tx(191-194)tx(191-191)tx(619-619)tx(628-628)tx(629-629)tx(630-630)tx(631-631)tx(632-632)tx(633-633)tx(634-634)tx(649-649)tx(799-799)tx(899-899)tx(949-949)tx(1007-1011)tx(1007-1007)newit(888-888)it(731-731)it(876-876)it(910-910)it(915-915)test/functional/test_framework/messages.py (1)
COutPoint(433-451)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/exceptions.cpp
[error] 30-30: Clang format differences found. Code formatting does not match the expected style.
src/governance/object.cpp
[error] 75-156: Clang format differences found. Code formatting does not match the expected style.
src/governance/vote.cpp
[error] 99-102: Clang format differences found. Code formatting does not match the expected style.
src/governance/classes.cpp
[error] 209-218: Clang format differences found. Code formatting does not match the expected style.
src/governance/governance.cpp
[error] 1132-1168: Clang format differences found. Code formatting does not match the expected style.
src/rpc/masternode.cpp
[error] 567-643: Clang format differences found. Code formatting does not match the expected style.
🔇 Additional comments (30)
src/util/string.h (1)
60-64: Well-implemented utility function for string padding.The new
PadStringfunction provides a clean and reusable way to pad strings with spaces to reach a target length. The implementation is concise and handles both left and right padding with a clear default behavior. The[[nodiscard]]attribute appropriately encourages proper use of the return value.src/txmempool.cpp (4)
688-689: Good optimization: caching transaction hash.Storing the hash value in a local variable avoids redundant hash computations in the rest of the function. This is especially valuable since hash calculation can be relatively expensive.
788-789: Good optimization: caching transaction hash.Consistent with the pattern established earlier, this change stores the hash in a local variable to avoid redundant calculations.
1020-1021: Good optimization: caching transaction hash.Continuing the pattern of hash caching to improve performance.
1024-1025: Consistent usage of cached transaction hash in error logs.The code consistently uses the cached hash value in logging statements, maintaining the optimization approach throughout the file.
Also applies to: 1045-1046, 1058-1059, 1067-1068
src/governance/vote.cpp (1)
99-102: Code formatting mismatch
Clang-format reported differences around the updatedToStringmethod. Please run:clang-format -i src/governance/vote.cppand ensure the file matches project style to resolve the GitHub Actions failure.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 99-102: Clang format differences found. Code formatting does not match the expected style.
src/llmq/snapshot.cpp (2)
26-26: Streamlined UniValue object initialization
Good refactor: replacing two-stepUniValue obj; obj.setObject();with direct constructionUniValue obj(UniValue::VOBJ);enhances consistency and readability.
45-45: Streamlined UniValue object initialization
Same improvement inCQuorumRotationInfo::ToJson(). Consistent with the rest of the codebase.src/evo/dmnstate.cpp (2)
40-40: Streamlined UniValue object initialization
Replaced default construction plussetObject()withUniValue obj(UniValue::VOBJ);inCDeterministicMNState::ToJson(). Improves uniformity across JSON serializers.
71-71: Streamlined UniValue object initialization
Applied the same direct-constructor pattern inCDeterministicMNStateDiff::ToJson(). Matches project conventions.src/evo/mnhftx.h (1)
52-52: Streamlined UniValue object initialization
RefactoredMNHFTx::ToJson()to construct the UniValue object withUniValue::VOBJdirectly. Aligns with other JSON serializers in the evo module.src/evo/simplifiedmns.cpp (2)
75-75: Streamlined UniValue object initialization
UpdatedCSimplifiedMNListEntry::ToJson()to useUniValue obj(UniValue::VOBJ);instead ofobj.clear()/obj.setObject(). Improves consistency.
241-241: Streamlined UniValue object initialization
Same pattern applied inCSimplifiedMNListDiff::ToJson(). Good uniform refactor.src/masternode/meta.cpp (1)
132-133: Good refactoring of string formatting.Replacing the
std::ostringstreamapproach withstrprintfmakes the code cleaner and more readable while maintaining the same functionality. This aligns with the PR's goal of simplifying string handling.src/governance/classes.cpp (3)
27-27: Well-structured error message with function context.Good use of
__func__to include the function name in error messages, making them more informative for debugging.
177-179: Improved error messaging pattern with separate log and throw.Consistently using
strprintfto format the error message, logging it, and then throwing it as an exception is a good pattern that improves code maintainability.
32-62: Consistent error message formatting.All error messages in this block now follow a consistent pattern using
strprintfwith__func__as the first parameter, improving readability and maintainability.src/test/util_tests.cpp (1)
1831-1848: Good test coverage for the new PadString utility function.The test case covers all the important scenarios:
- Default left padding behavior
- Explicit left and right padding options
- Handling when padding size ≤ string size
- Edge cases like empty strings
This ensures the functionality works as expected across various inputs.
src/evo/deterministicmns.cpp (1)
47-47: Improved UniValue initialization.Using direct construction with the type (
UniValue(UniValue::VOBJ)) instead of a two-step initialization process is more concise and potentially more efficient by avoiding an extra function call.src/governance/exceptions.cpp (1)
6-6: Good addition of tinyformat.h headerIncluding tinyformat.h is necessary to support the subsequent use of strprintf. This is a good dependency management practice.
src/rpc/evo.cpp (5)
683-689: Improved string emptiness checkGood change to use the more idiomatic
!str.empty()instead ofstr != ""for checking if a string is empty. This is more semantically clear and potentially more efficient as it directly checks the string's size property rather than performing a string comparison.
697-699: Improved string emptiness checkConsistent use of
.empty()method for checking string emptiness. This follows modern C++ idioms.
1109-1115: Improved string emptiness checkGood continuation of the pattern to use
.empty()for string emptiness checks throughout the codebase.
1119-1121: Improved string emptiness checkConsistent application of the
.empty()method for string emptiness checks.
1125-1131: Improved string emptiness checkConsistent application of the
.empty()method for string emptiness checks, completing the pattern throughout this file.src/governance/governance.cpp (1)
1151-1153: Consistent use of strprintf for message formattingThis change maintains the consistent pattern of using
strprintfwith__func__for log and error messages throughout the governance code.src/rpc/masternode.cpp (4)
573-582: Replaceostringstreamwithstrprintf+PadStringin 'full' mode
The newstrprintf+PadStringapproach is more concise and readable than manual stream formatting. Filter logic remains correct.
584-591: Streamline 'info' mode formatting
UsingstrprintfandPadStringhere cleans up the code and aligns with other modes. Filter usage is consistent.
593-607: Unify 'json/recent/evo' mode formatting and filter
The refactored construction ofstrInfoviastrprintfimproves readability, and the subsequent UniValue assembly is clear. Filter application is unchanged and correct.
644-647: Consistent.empty()check in 'status' mode
Good use of!strFilter.empty()and checking both status and outpoint.
| auto dmn = tip_mn_list.GetMNByCollateral(masternodeOutpoint); | ||
| int voteWeight = dmn != nullptr ? GetMnType(dmn->nType).voting_weight : 0; | ||
| std::ostringstream ostr; | ||
| ostr << masternodeOutpoint.ToStringShort() << ":" | ||
| << nTime << ":" | ||
| << CGovernanceVoting::ConvertOutcomeToString(GetOutcome()) << ":" | ||
| << CGovernanceVoting::ConvertSignalToString(GetSignal()) << ":" | ||
| << voteWeight; | ||
| return ostr.str(); | ||
| return strprintf("%s:%s:%s:%s:%s", | ||
| masternodeOutpoint.ToStringShort(), nTime, | ||
| CGovernanceVoting::ConvertOutcomeToString(GetOutcome()), CGovernanceVoting::ConvertSignalToString(GetSignal()), | ||
| voteWeight); | ||
| } |
There was a problem hiding this comment.
Mismatched format specifiers in strprintf
The format string ("%s:%s:%s:%s:%s") uses %s for all placeholders, but nTime and voteWeight are integer types. This is undefined behavior. Please update the specifiers to the correct integer formats (for example, %lld or %" PRI64d " for nTime and %d for voteWeight), or convert these values to strings (e.g., via std::to_string) before passing them to strprintf.
| } | ||
|
|
||
| LogPrint(BCLog::GOBJECT, /* Continued */ | ||
| "CSuperblock::ParsePaymentSchedule -- i = %d, amount string = %s, nAmount = %lld, proposalHash = %s\n", | ||
| "CSuperblock::%s -- i = %d, amount string = %s, nAmount = %lld, proposalHash = %s\n", __func__, | ||
| i, vecPaymentAmounts[i], nAmount, proposalHash.ToString()); |
There was a problem hiding this comment.
Fix code formatting issues.
The Clang Diff Format Check shows formatting errors in this section, likely related to line indentation or breaks.
Please run clang-format on this file section or manually adjust the formatting to comply with the project's style guidelines.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 209-218: Clang format differences found. Code formatting does not match the expected style.
Additional Information
Dependency for refactor: section off masternode network information to
MnNetInfo, set some groundwork for managing multiple entries #6627We use
std::ostringstreaminmasternodelistto pad values usingstd::setw(source) for consistency, this was fine because we didn't plan on removing anything from the stream after it was added.But in dash#6627, we intend to print multiple addresses and need to remove excess padding after the last entry, which can be trivially done with a
pop_back()(source) but that cannot be done with astd::ostringstream. The best that can be done is rewinding the cursor but that doesn't change its contents and at most lets you overwrite it (source), which is fine when you do have something to overwrite it with but this isn't always the case.To get around this,
PadString()has been implemented andstd::ostringstreamusage has been replaced with constructing a string withstrprintf.Breaking Changes
None expected.
Checklist