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

Removes Default and pub from CumulativeOffset #33840

Merged

Conversation

brooksprumo
Copy link
Contributor

Problem

  1. CumulativeOffset and its fields are marked public, but are only used within accounts_hash.rs.
  2. With Optimize account hash CumulativeOffset index from vec to 2-element array #33839, the type of index will change to a two-element array. A default doesn't necessarily make sense, as we'll always need at least one valid index to be set.

Summary of Changes

Remove pub and Default.

@brooksprumo brooksprumo self-assigned this Oct 24, 2023
}

pub trait ExtractSliceFromRawData<'b, T: 'b> {
trait ExtractSliceFromRawData<'b, T: 'b> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this trait was public, and the extract function takes a CumulativeOffset, that meant CumulativeOffset needed to be public. But extract is only called in this file, so the trait does not need to be public either.

@brooksprumo brooksprumo marked this pull request as ready for review October 24, 2023 17:01
@brooksprumo brooksprumo requested a review from HaoranYi October 24, 2023 17:01
HaoranYi
HaoranYi previously approved these changes Oct 24, 2023
Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #33840 (b3dd36d) into master (6470544) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 100.0%.

❗ Current head b3dd36d differs from pull request most recent head 00ce0c2. Consider uploading reports for the commit 00ce0c2 to get more accurate results

@@           Coverage Diff           @@
##           master   #33840   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         809      809           
  Lines      217712   217627   -85     
=======================================
- Hits       178295   178250   -45     
+ Misses      39417    39377   -40     

@brooksprumo
Copy link
Contributor Author

Rebased and force-pushed to address merge conflict. No actual changes were made.

@brooksprumo brooksprumo requested a review from HaoranYi October 24, 2023 18:29
@brooksprumo brooksprumo added the automerge Merge this Pull Request automatically once CI passes label Oct 24, 2023
@mergify mergify bot merged commit 612e8e8 into solana-labs:master Oct 24, 2023
18 checks passed
@brooksprumo brooksprumo deleted the cumulative-offset/no-default branch October 24, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants