refactor: Rename static constants - #7120
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7120 +/- ##
=========================================
- Coverage 82.1% 82.0% -0.0%
=========================================
Files 1010 1010
Lines 76224 76151 -73
Branches 7419 7418 -1
=========================================
- Hits 62553 62479 -74
- Misses 13671 13672 +1
🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
mathbunnyru
left a comment
There was a problem hiding this comment.
While we're doing this, let's also unify the order in which we declare static, constexpr and type or auto to be: static constexpr type|auto
It's easy to fix via replace, I suppose, and touches exactly the same lines, so better be done here.
I initially thought this would be a separate PR later but i agree we may as well do it here. |
ximinez
left a comment
There was a problem hiding this comment.
I used Copilot to help find changes that aren't just renames, and it found these.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
ximinez
left a comment
There was a problem hiding this comment.
It occurred to me overnight to ask if we could change the format of local constexprs & statics to be camelBack style (e.g. ManagerImp::instance() would be inst instead of kInst, but this got merged before I had a chance. Not a deal breaker.
I also wanted to share that I asked Claude yesterday to:
Find any changes in #7120 that aren't limited to renaming a variable, changing formatting and whitespace, or reordering the constexpr keyword in variable declarations. Take the time to review all changes.
This is the report it gave me back. Since we squash, the concerns about intermediate commits are moot. Otherwise, it's pretty neat.
Substantive Changes in PR #7120
1. Bugs Introduced by the Rename Script — Fixed by Commit 06d3d0f
The rename automation and a bad merge-conflict resolution in earlier commits introduced four real bugs,
which commit 06d3d0f
("Fix introduced issues unrelated to renames") corrected. The net PR diff is correct for all of
these, but the intermediate history is messy.
include/xrpl/tx/transactors/bridge/XChainBridge.h — Four wrong ConsequencesFactoryType values
Flagged by @ximinez in 4 review comments.
Merge commit a49d8d1a6 accidentally reset every XChain transactor's kConsequencesFactory to
Normal. The values that were wrong mid-PR:
| Class | Correct value | Broken intermediate |
|---|---|---|
XChainClaim |
Blocker |
Normal |
XChainCommit |
Custom |
Normal |
XChainAddClaimAttestation |
Blocker |
Normal |
XChainAddAccountCreateAttestation |
Blocker |
Normal |
These affect transaction queuing behavior and are semantically significant. 06d3d0f restored the
correct values, so the final net diff is a pure rename.
include/xrpl/tx/applySteps.h — Spurious {} initializer
Flagged by @ximinez in 1 review comment.
The rename script changed XRPAmount fee_; to XRPAmount fee_{};, adding value-initialization where
none existed. 06d3d0f removed it. Net PR diff for this file is zero (unchanged from base).
include/xrpl/resource/detail/Tuning.h — Dropped static
The rename script dropped static from a constant declaration and misplaced constexpr. 06d3d0f
restored static. Net diff is a pure rename.
src/libxrpl/nodestore/ManagerImp.cpp — Non-mechanical rename
A local static k_ was renamed to kInst rather than a strict kFooBar-style conversion. This is
a rename, but to a semantically new name.
2. Unrelated Changes Pulled In via develop Merges
The PR merged develop twice during its lifetime, bundling in the following unrelated changes.
src/xrpld/rpc/detail/Role.cpp — Security fix in Forwarded header parsing
The forwardedFor() function was substantially refactored (from PR #7126 in develop):
- Added boundary-checking logic so
"for="only matches at a directive boundary, not inside tokens
like"before=" - Changed to a
while(true)loop with explicit boundary checking - Changed
kFOR_STRtype fromstd::stringtostd::string_view
The rename (kFOR_STR → kForStr) is layered on top of this security/correctness fix.
src/libxrpl/tx/transactors/lending/LoanPay.cpp — Logic changes from develop
Includes a fix for a funds-are-conserved assertion and a rename of the feature flag
fixSecurity3_1_3 → fixCleanup3_1_3.
.clang-tidy — Intentional rule changes (the point of the PR)
StaticVariableCase:UPPER_CASE→camelBack(dropping thekprefix requirement)ConstexprVariableCase:UPPER_CASE→camelBack(dropping thekprefix requirement)
Infrastructure / CI
.github/workflows/build-nix-image.yml— New nix-based Dockerfile CI workflowconan/global.conf— Added retry settings for Conan downloads.pre-commit-config.yaml— Updatedcspellexclude pattern.github/scripts/rename/config.shanddocs.sh— Updated to reference the renamed constant
kConfigLegacyName(waskCONFIG_LEGACY_NAME)
Bottom Line
The final state of the PR is clean — all constant values and runtime behavior are preserved relative
to the base branch. However, the intermediate history contained genuinely dangerous bugs (the wrong
ConsequencesFactoryType values would have misclassified XChain transactions in the queue), and the
diff bundles several unrelated changes that arrived via develop merges, making it harder to review
in isolation.
Are we going back to Hungarian notation? |
High Level Overview of Change
In #6571 we merged clang-tidy's
readability-identifier-namingcheck but usedkALL_CAPSfor statics and global constants. This style did not receive too much love andkCamelCaseis here to replace it.Other than the renames this PR also reorders 'static constexpr Type' and adds static to missing constexpr constants that can be made static.
API Impact
No impact.