Remove checkData from copy constructor and assignment operator in Fields

[d7919]

This allows us to copy and assign from empty fields.

[ZedThree]

I think we test for this behaviour, so you'll likely need to remove those tests as well

[d7919]

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.

[codecov-io]

Codecov Report

Merging #1533 into next will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
src/field/field2d.cxx	78.63% <ø> (-0.19%)	⬇️
src/field/fieldperp.cxx	100% <ø> (ø)	⬆️
src/field/field3d.cxx	57.88% <ø> (-0.17%)	⬇️
tests/unit/field/test_field3d.cxx	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a68fee...168aa54. Read the comment docs.

[ZedThree]

Not sure why CodeCov has chimed in!

Any major objections to this? I think this is the right thing to do (see #1385)

[d7919]

One possible objection is operator= with mixed types (e.g 3d = 2d) have to do checkdata but same types (3d=3d) don't.

[johnomotani]

I agree this is the right thing to do 👍

[ZedThree]

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.

[d7919]

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 isAllocated type check though.

[bendudson]

I think having an isAllocated check, and invalidating the LHS if the RHS is invalid is probably the most consistent approach. I have some worry that not checking for invalid data will make tracking down bugs harder, but a user can always add calls to checkData for debugging.

[d7919]

I think having an isAllocated check, and invalidating the LHS if the RHS is invalid is probably the most consistent approach. I have some worry that not checking for invalid data will make tracking down bugs harder, but a user can always add calls to checkData for debugging.

I hope this shouldn't impact debugging too much as we still rigorously checkData on the input and output of most operations so at the point of doing any calculation with the fields we should catch this.

[d7919]

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.

[ZedThree]

Merging this now to immediately update #1345, but we may want to revisit mixed-type assignment for uninitialised fields