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

Refactor - Bank::compute_active_feature_set() #32872

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Aug 17, 2023

Problem

This PR was split from #32172.

Currently Bank::compute_active_feature_set() does not just compute the active feature set as the name implies, but also writes the result back into self.

Summary of Changes

Bank::compute_active_feature_set() now returns the new feature set and the difference to the current one. The overwriting of self.feature_set was factored out into the call sites.

@Lichtso Lichtso requested a review from pgarg66 August 17, 2023 14:45
@dmakarov dmakarov self-requested a review August 17, 2023 14:45
@Lichtso Lichtso requested review from dmakarov and removed request for dmakarov August 17, 2023 14:47
@Lichtso Lichtso added the runtime Issues related to runtime, BPF, and LLVM label Aug 17, 2023
@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 17, 2023

@dmakarov Wanted to add labels, accidentally removed a reviewer. Please ignore, and proceed with the review.

@@ -7982,7 +7984,10 @@ impl Bank {
}

// Compute the active feature set based on the current bank state, and return the set of newly activated features
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems the comment becomes outdated by the change, doesnt' it?

@Lichtso Lichtso force-pushed the refactor/bank_compute_active_feature_set branch from a03fc32 to c66c76a Compare August 17, 2023 15:17
@dmakarov
Copy link
Contributor

LGTM.

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #32872 (c66c76a) into master (fea5460) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #32872   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         785      785           
  Lines      212412   212412           
=======================================
+ Hits       174212   174261   +49     
+ Misses      38200    38151   -49     

@Lichtso Lichtso merged commit c5a251e into master Aug 17, 2023
@Lichtso Lichtso deleted the refactor/bank_compute_active_feature_set branch August 17, 2023 17:14
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 21, 2023
Returns the feature set instead of overwriting it inside Bank::compute_active_feature_set().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime Issues related to runtime, BPF, and LLVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants