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

Uses impl_crds_entry! for SnapshotHashes #31032

Conversation

brooksprumo
Copy link
Contributor

Problem

The manual implementation of get_entry() for SnapshotHashes can be replaced by impl_crds_entry!. Since it currently isn't, I end up rereading the impl to understand how the manual and macro impls are different, only to learn they are not different.

Summary of Changes

Use impl_crds_entry! to implement get_entry() for SnapshotHashes.

@brooksprumo brooksprumo self-assigned this Apr 3, 2023
@brooksprumo brooksprumo force-pushed the snapshot-gossip/snapshot-hashes-crds-entry branch from 0e37b57 to 0fac443 Compare April 3, 2023 20:53
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #31032 (0fac443) into master (e8ea722) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #31032     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         728      728             
  Lines      205583   205578      -5     
=========================================
- Hits       167711   167678     -33     
- Misses      37872    37900     +28     

@brooksprumo brooksprumo marked this pull request as ready for review April 3, 2023 23:41
@brooksprumo brooksprumo requested a review from behzadnouri April 3, 2023 23:41
impl_crds_entry!(
IncrementalSnapshotHashes,
CrdsData::IncrementalSnapshotHashes(incremental_snapshot_hashes),
incremental_snapshot_hashes
);

impl<'a, 'b> CrdsEntry<'a, 'b> for &'a SnapshotHashes {
Copy link
Contributor

Choose a reason for hiding this comment

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

This manual implementation was because up until #20421, this was called SnapshotHash vs SnapshotHashes; so the existing macro wouldn't work here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the link and context 🙏

@brooksprumo brooksprumo merged commit aa8d7de into solana-labs:master Apr 4, 2023
@brooksprumo brooksprumo deleted the snapshot-gossip/snapshot-hashes-crds-entry branch April 4, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants