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

Replace Require strings with lambdas #1755

Merged
merged 3 commits into from
Feb 11, 2023
Merged

Replace Require strings with lambdas #1755

merged 3 commits into from
Feb 11, 2023

Conversation

Bushstar
Copy link
Member

The adding of Require impacted performance as strings were always created regardless of whether the conditional was true or false. This PR updates all Require statements not with the exception of mn_checks.

Jouzo
Jouzo previously approved these changes Feb 10, 2023
@Jouzo Jouzo self-requested a review February 10, 2023 15:16
@prasannavl
Copy link
Member

Leaving these for context:

image

We shouldn't have any observable difference from lambas on regular checks that isn't run through expensive calculations like decimal conversions, since it's effectively the same (if anything an additional function call, but that would be micro-opt that we don't need to be concerned about here)

Merging for now, since it's already reviewed and verified, on all tests and PR does cover some impactful areas. However, need to evaluate long term maintainability of this later.

@prasannavl prasannavl merged commit 77b7326 into master Feb 11, 2023
@prasannavl prasannavl deleted the require-add-lamdas branch February 11, 2023 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants