-
Notifications
You must be signed in to change notification settings - Fork 8
Extend clone-on-write scenarios #1222
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
base: main
Are you sure you want to change the base?
Conversation
| //} | ||
| auto weak = v.weak_from_this(); | ||
| if (weak.expired()) { | ||
| Become(Valuable(v.Clone())); |
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.
Everything looks good! The changes to extend clone-on-write scenarios are well-implemented and follow the existing code patterns.
e0f6de6 to
d904356
Compare
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.
Everything looks good! The changes to extend clone-on-write scenarios are well-implemented and follow the existing code patterns.
| if (inst) | ||
| exp = inst; | ||
| else { | ||
| exp.reset(v.Clone()); | ||
| //auto weak = v.weak_from_this(); | ||
| //if (weak.expired()) { | ||
| // Become(Valuable(v.Clone())); | ||
| //} else { | ||
| // auto locked = weak.lock(); | ||
| // exp = std::const_pointer_cast<Valuable>(locked); | ||
| //} | ||
| auto weak = v.weak_from_this(); | ||
| if (weak.expired()) { | ||
| Become(Valuable(v.Clone())); | ||
| } else { | ||
| auto locked = weak.lock(); | ||
| exp = std::const_pointer_cast<Valuable>(locked); | ||
| } |
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.
Memory Management Issue: The clone-on-write implementation in operator= is causing memory access violations and test failures. The current implementation using weak_from_this() appears to be creating dangling references or invalid memory access patterns.
else {
auto weak = v.weak_from_this();
if (weak.expired()) {
Become(Valuable(v.Clone()));
} else {
auto locked = weak.lock();
exp = std::const_pointer_cast<Valuable>(locked);
}
}This approach is causing 15 out of 34 tests to fail with memory access violations. Consider reverting to the previous direct cloning approach or implementing a more robust shared_ptr management strategy that doesn't cause these issues.
| @@ -149,6 +149,10 @@ namespace omnn::math { | |||
| } | |||
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.
CI Build Failures: The changes to the clone-on-write implementation are causing widespread test failures (15 out of 34 tests failing) in the CI build. The memory access violations in tests like ViewMatrix_test indicate that the weak_ptr approach is creating memory corruption issues.
The current implementation is causing failures across core mathematical operations (Exponentiation, Fraction, Integer, Product, Sum) which suggests a fundamental issue with the memory management approach rather than an isolated bug.
010d008 to
edef2a4
Compare
Extend clone-on-write scenarios