-
Notifications
You must be signed in to change notification settings - Fork 95
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
Remove checkData from copy constructor and assignment operator in Fields #1533
Conversation
I think we test for this behaviour, so you'll likely need to remove those tests as well |
It passed all the unit tests locally! Edit Ah, it probably depends on the check level. |
…nt operator in Fields" This partially reverts commit 732fdb0.
Codecov Report
@@ Coverage Diff @@
## next #1533 +/- ##
==========================================
- Coverage 43.06% 43.05% -0.02%
==========================================
Files 201 201
Lines 27257 27252 -5
==========================================
- Hits 11738 11733 -5
Misses 15519 15519
Continue to review full report at Codecov.
|
Not sure why CodeCov has chimed in! Any major objections to this? I think this is the right thing to do (see #1385) |
One possible objection is operator= with mixed types (e.g 3d = 2d) have to do checkdata but same types (3d=3d) don't. |
I agree this is the right thing to do 👍 |
For the mixed types assignment, I think we need at least the check the rhs is allocated (else the copy won't work), but we could change what we do with that information, e.g. if the rhs is not allocated, deallocate the lhs and return. Otherwise, do the copy without checking if the rhs is finite. |
Yes I thought about that a bit but decided for now that I'd just do the simplest thing (i.e. leave the checks in place). Happy to replace those with an |
I think having an |
I hope this shouldn't impact debugging too much as we still rigorously |
|
||
|
||
/// Check that the data is allocated | ||
ASSERT1(rhs.isAllocated()); |
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.
Instead of ASSERT
here we could just choose to return the unallocated Field3D
so that we allow assignment from unallocated fields -- I'm not sure that would be good but is an option.
Merging this now to immediately update #1345, but we may want to revisit mixed-type assignment for uninitialised fields |
This allows us to copy and assign from empty fields.