-
Notifications
You must be signed in to change notification settings - Fork 140
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
debug execution: implement actor redirects in engine #605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a very reasonable way to do this.
.as_ref() | ||
.map(|v| v.iter().cloned().collect()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not overriding anything, we might as well just have an empty hashmap.
.as_ref() | |
.map(|v| v.iter().cloned().collect()); | |
.as_ref() | |
.unwrap_or_default().iter().cloned().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, do you want to drop the option from the data structure?
I thought it would be faster to None
check instead of a get in an empty map, but if there is no difference we might as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option prevents us from hashing on access when there are no redirects set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so lets keep it and decline steb's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option prevents us from hashing on access when there are no redirects set.
Rust is smarter than that https://github.com/rust-lang/hashbrown/blob/f3a242fb48e73df704d27c9722e30ebda5dd8e4f/src/map.rs#L1099-L1104
@@ -223,8 +234,19 @@ impl Engine { | |||
Ok(()) | |||
} | |||
|
|||
fn with_redirect<'a>(&'a self, k: &'a Cid) -> &'a Cid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this explicit lifetime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i refuses to compile without it.
This was easier and cleaner than I expected, so no need for generics. Or wrapping the engine. KISS. |
6693eba
to
45a9e9e
Compare
rebased on master for the logging fix. |
Codecov Report
@@ Coverage Diff @@
## master #605 +/- ##
=======================================
Coverage 51.39% 51.39%
=======================================
Files 120 120
Lines 9512 9512
=======================================
Hits 4889 4889
Misses 4623 4623 |
@@ -116,6 +116,9 @@ pub struct NetworkConfig { | |||
/// | |||
/// DEFAULT: The price-list for the current network version. | |||
pub price_list: &'static PriceList, | |||
|
|||
/// Actor redirects for debug execution | |||
pub actor_redirect: Option<Vec<(Cid, Cid)>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to make this an option either. An empty vector is free.
Now that master is M2, I will merge this to |
This seems like the simplest way to do it, without getting into a mess trying to implement intercepting or generic engines.