[#1178] Add loom test setup #1194
Conversation
|
@mox692 Thanks you for the pull requests and taking care of loom! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
==========================================
- Coverage 77.87% 77.84% -0.04%
==========================================
Files 360 360
Lines 38833 38843 +10
Branches 816 816
==========================================
- Hits 30242 30238 -4
- Misses 7944 7958 +14
Partials 647 647
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
elfenpiff
left a comment
elfenpiff
left a comment
There was a problem hiding this comment.
I reviewed it and you implemented it exactly how we intended to integrate loom into iceoryx2 - well done, thank you!
Since it is a crucial part in iceoryx2 I ask @elBoberido that he reviews the PR as well and approves it. Then we can merge it.
elBoberido
left a comment
elBoberido
left a comment
There was a problem hiding this comment.
Looks good. Just some questions regarding no_std and documentation. Nothing blocking if no_std is still possible, which I assume.
|
|
||
| impl<T> LazySingleton<T> { | ||
| /// Creates a new [`LazySingleton`] where the underlying value is not yet initialized. | ||
| #[cfg(not(all(test, loom)))] |
There was a problem hiding this comment.
We want to make iceoryx2 no_std. Has this a negative effect on this goal?
There was a problem hiding this comment.
@mox692 @elBoberido Is there a chance to wait for #1156 before proceeding with this? It's hard to tell how those changes will interfere with this.
I am actively working to get it merged now. Pending reviews it should be in sometime this week...
There was a problem hiding this comment.
#1156 Is merged. If you can rebase this PR and adapt as required, should be good to go.
| #[cfg(all(test, loom))] | ||
| pub fn new() -> Self { | ||
| Self { | ||
| data: UnsafeCell::new(None), | ||
| is_initialized: IoxAtomicBool::new(false), | ||
| is_finalized: IoxAtomicBool::new(false), | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we document that loom cannot handle const functions somewhere? I mean in case we introduce a new function and loom breaks. A good option would be to add the error message and the fix to FAQ_ICEORYX_DEVS.md.
| #[cfg(not(all(test, loom)))] | ||
| static COUNTER: IoxAtomicU64 = IoxAtomicU64::new(0); | ||
| #[cfg(all(test, loom))] | ||
| static COUNTER: std::sync::LazyLock<IoxAtomicU64> = std::sync::LazyLock::new(|| { |
There was a problem hiding this comment.
@orecham I guess this is not an issue for no_std as long as iceoryx2 is build with std support.
There was a problem hiding this comment.
This would work fine, but would mean only the std build is being validated - maybe OK, at least for the first steps?
|
Thank you for the review!
Yes good catch, I think this PR should probably be modified so that the loom tests are gated behind the std feature. At this moment, loom does not support no_std environment. Would it be acceptable to enable loom support only for the targets that are built with |
@mox692 Yup, makes sense to me :) |
4 participants
Notes for Reviewer
This PR adds the setup for loom tests. The actual loom tests will be added in follow-up PRs.
A couple of notes:
cfgflag. I chosecfgbecause I assume we don’t want to introduce a new Cargo feature across several crates just for this purpose. Usingcfgavoids having to touchCargo.tomlin many places.staticvariables that would otherwise cause a runtime error when executed under loom.Pre-Review Checklist for the PR Author
Convert to draft)SPDX-License-Identifier: Apache-2.0 OR MITiox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Relates to #1178.