Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1336 +/- ##
==========================================
- Coverage 56.40% 56.39% -0.02%
==========================================
Files 141 141
Lines 20126 20127 +1
Branches 4998 4998
==========================================
- Hits 11353 11350 -3
- Misses 5883 5886 +3
- Partials 2890 2891 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| impl std::convert::From<u64> for SimulationTime { | ||
| fn from(val: u64) -> Self { | ||
| Self(std::time::Duration::from_nanos( | ||
| val * SIMTIME_ONE_NANOSECOND, | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this would be less confusing as something like impl SimulationTime { fn from_c_sim_time() -> Self }. If we have places in the code like 1234.into() or SimulationTime::from(1234), it's not clear that the argument is supposed to be in nanoseconds without reading the implementation of SimulationTime.
There was a problem hiding this comment.
Good point; done.
| /// Whether currently running on a live Worker. | ||
| pub fn is_alive() -> bool { | ||
| unsafe { cshadow::worker_isAlive() != 0 } | ||
| } | ||
|
|
||
| /// Current simulation time, or None if not running on a live Worker. | ||
| pub fn current_time() -> Option<SimulationTime> { | ||
| if !Worker::is_alive() { | ||
| return None; | ||
| } | ||
| Some(SimulationTime::from(unsafe { | ||
| cshadow::worker_getCurrentTime() | ||
| })) | ||
| } | ||
|
|
||
| /// Id of the current worker thread, or None if not running on a live Worker. | ||
| pub fn thread_id() -> Option<i32> { | ||
| if !Worker::is_alive() { | ||
| return None; | ||
| } | ||
| Some(unsafe { cshadow::worker_getThreadID() }) | ||
| } |
There was a problem hiding this comment.
All of these C functions (worker_isAlive(), worker_getCurrentTime(), worker_getThreadID()) use the global worker, so I think they should also use the WORKER.with(|worker| {}),
There was a problem hiding this comment.
I think this would be a no-op. WORKER is an immutable Worker object. It itself isn't behind a RefCell.
There was a problem hiding this comment.
Okay I think I see what the plan is now. So WORKER would eventually contain all of the members from the C Worker struct, but all of the members will be (ref)cells. So for example WORKER will have members like object_alloc_counter: RefCell<Counter> and now: Cell<SimulationTime>. Is this correct?
| --whitelist-function "worker_getWritablePtr" | ||
| --whitelist-function "worker_getMutablePtr" | ||
| --whitelist-function "worker_flushPtrs" | ||
| --whitelist-function "worker_.*" |
There was a problem hiding this comment.
Do we want to blacklist some functions? We're including a bunch of GLib structs now, and it might cause issues later if we're generating static bindings for a library that we don't control (for example if GLib later adds a new struct member and our bindings would be incompatible with different GLib versions).
There was a problem hiding this comment.
I think the struct definitions that glib makes visible in header files are meant to be stable across versions, but yeah we can punt on these ones for now.
There was a problem hiding this comment.
I'm just worried that if we need to support distros that have different versions of GLib, then it might be difficult to provide bindings that are valid for both versions. I agree with the comments in your commit message, there are other ways we could work around this, but I think we should also try to avoid having to ever work with GLib types from Rust.
| } | ||
|
|
||
| pub fn name(&self) -> &str { | ||
| let slice = unsafe { std::ffi::CStr::from_ptr(cshadow::host_getName(self.chost)) }; |
There was a problem hiding this comment.
Maybe check that host_getName() doesn't return NULL since the C Host object allows the hostname to be NULL.
There was a problem hiding this comment.
I don't think it actually can be NULL, can it? You can't actually define a host in the config without giving it a name...?
Added an assertion to host_new to be sure.
There was a problem hiding this comment.
Within Shadow I don't think it is ever NULL, I was just thinking about within the scope of the Host type specifically since its constructor accepts a NULL value.
Not clear yet how best to handle these; avoid using them until we do. Some options * Avoid ever wrapping them; ideal but maybe difficult. * Just go ahead and wrap them. bindgen's static assertions *should* catch if any structs are a different size than expected. They won't catch if fields are reordered, but that's probably unlikely. * Wrap them as opaque types. This would also protect us from reordering, though we'd then need to create C accessors for those fields. * Dynamically generate bindings instead of checking them in. This would guarantee that the types match the types on the machine where it's built. It wouldn't help with distributed binaries, though.
SimulationTimeto help avoid mixing with other time types.Progress on #1301