Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Trivial] Remove unused constants from resource/Fees.h #4856

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

scottschurr
Copy link
Collaborator

High Level Overview of Change

While reviewing other code I stumbled across a number of constants defined in resource/Fees.h that are unused anywhere in the code base. They did no harm, but it seems untidy to leave them lying around. This commit removes the unused constants.

I also tidied up some messiness left by clang-format.

Type of Change

  • Refactor (non-breaking change that only restructures code)

@scottschurr
Copy link
Collaborator Author

This pull request had become particularly stale. Since there have been no active reviews yet, I rebased the pull request to the latest develop.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.4%. Comparing base (a39720e) to head (47cef90).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #4856     +/-   ##
=========================================
- Coverage     71.4%   71.4%   -0.0%     
=========================================
  Files          796     796             
  Lines        67042   67042             
  Branches     10863   10866      +3     
=========================================
- Hits         47844   47843      -1     
- Misses       19198   19199      +1     

see 6 files with indirect coverage changes

Impacted file tree graph

@intelliot intelliot requested a review from ximinez July 25, 2024 23:05
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised to see that none of the removed variables are defined in Fees.cpp. It's not just that they aren't used in code, they're not even defined, so if anyone did try to use one, they'd get a linker error.

Since this is so old, I think it would be ok to rebase it on current develop, instead of needing to do the work of a merge that includes the reorginization. Do you want to do it, or should I?

@scottschurr
Copy link
Collaborator Author

I'll do the rebase. I should be able to get it done today.

…sed-charge

* upstream/develop:
  fix: Fix NuDB build error via Conan patch (5061)
  Disallow filtering account_objects by unsupported types (5056)
  chore: Add comments to SignerEntries.h (5059)
  chore: Rename two files from Directory* to Dir*: (5058)
  Update BUILD.md after PR 5052 (5067)
  Add xrpld build option and Conan package test (5052)
  chore: remove repeat words (5053)
  fix CTID in tx command returns invalidParams on lowercase hex (5049)
  Invariant: prevent a deleted account from leaving (most) artifacts on the ledger. (4663)
  Bump codecov plugin version to version 4.5.0 (5055)
  fix "account_nfts" with unassociated marker returning issue (5045)
  fixInnerObjTemplate2 amendment (5047)
  Set version to 2.3.0-b1
  Ignore restructuring commits (4997)
  Recompute loops (4997)
  Rewrite includes (4997)
  Rearrange sources (4997)
  Move CMake directory (4997)
  Add bin/physical.sh (4997)
  Prepare to rearrange sources: (4997)
@ximinez
Copy link
Collaborator

ximinez commented Jul 29, 2024

Per offline discussion, the merge was actually easy, so I pushed it.

@ximinez
Copy link
Collaborator

ximinez commented Jul 29, 2024

Presumably, this will be ready for merge once CI passes (unless it needs to be caught up to develop again).

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 30, 2024
@ximinez ximinez self-assigned this Jul 30, 2024
@ximinez ximinez merged commit b9b75dd into XRPLF:develop Jul 30, 2024
19 of 20 checks passed
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Low Priority Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants