Adjust ok_or and map_or to _else for lazy computation #14764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 2h total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
gelash
added
CICD:run-execution-performance-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14764 +/- ##
=========================================
- Coverage 60.1% 60.0% -0.1%
=========================================
Files 856 856
Lines 210845 210892 +47
=========================================
- Hits 126742 126737 -5
- Misses 84103 84155 +52 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gelash
changed the title
gelash
requested review from
alinush,
rex1fernando,
msmouse,
lightmark,
grao1991,
JoshLind,
bchocho,
gregnazario,
banool,
zekun000,
sasha8,
ibalajiarun,
runtian-zhou,
danielxiangzl,
georgemitenkov,
davidiw,
wrwg,
vgao1996 and
0xmaayan
as code owners
|
A pass for |
|
there is also straight up or( that I didn't look for yet |
msmouse
approved these changes
Sep 26, 2024
alinush
approved these changes
Sep 26, 2024
igor-aptos
requested changes
Sep 26, 2024
| let mut prev_modified_keys = last_input_output | ||
| .modified_keys(idx_to_execute) | ||
| .map_or(HashMap::new(), |keys| keys.collect()); | ||
| let mut prev_modified_keys = match last_input_output.modified_keys(idx_to_execute) { |
There was a problem hiding this comment.
why not just map_or_else?
There was a problem hiding this comment.
issues with type deduction somehow
There was a problem hiding this comment.
if there's error you just need to annotate the types like keys.collect::<HashMap<,>>() but I also don't see errors when trying it
There was a problem hiding this comment.
I tried annotating and kept seeing weird errors, will try again
There was a problem hiding this comment.
worked out, must have been on some weird local state
| let output_prefix = options.move_sources.first().map_or("bytecode", |s| { | ||
| Path::new(s).file_name().unwrap().to_str().unwrap() | ||
| }); | ||
| let output_prefix = options.move_sources.first().map_or_else( |
There was a problem hiding this comment.
const literals are cheaper than functions?
There was a problem hiding this comment.
true, missed this one, usually the linter would complain in this case. will revert.
There was a problem hiding this comment.
you have this in multiple places as far as I can see
There was a problem hiding this comment.
will do a pass. For sure left a bunch unchanged and got linter errors and reverted on some. string literal def can stay in or(
There was a problem hiding this comment.
I think I got all of them
zekun000
reviewed
Sep 26, 2024
| let mut prev_modified_keys = last_input_output | ||
| .modified_keys(idx_to_execute) | ||
| .map_or(HashMap::new(), |keys| keys.collect()); | ||
| let mut prev_modified_keys = match last_input_output.modified_keys(idx_to_execute) { |
There was a problem hiding this comment.
if there's error you just need to annotate the types like keys.collect::<HashMap<,>>() but I also don't see errors when trying it
gelash
force-pushed
the
gelash/orfix
branch
2 times, most recently
from
287eb78 to
54c63b7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
igor-aptos
approved these changes
Oct 11, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Audit and adjust the uses, e.g. where there is error preparation or string formatting on the unhappy path, to make sure it doesn't execute eagerly on the happy path.
How Has This Been Tested?
passes existing
Type of Change
Which Components or Systems Does This Change Impact?