Safer C++ DataContainer data type handling updated #1210
Conversation
…o data-cont-templ
| if (this != &to_copy) { | ||
| if (static_cast<void*>(this) != static_cast<const void*>(&to_copy)) { | ||
| // Try to cast to NiftiImageData. | ||
| const NiftiImageData<dataType> * const nii_ptr = dynamic_cast<const NiftiImageData<dataType> * const >(&to_copy); |
There was a problem hiding this comment.
Are the following lines supposed to work also if dataType != float?
There was a problem hiding this comment.
no idea, this is Richard's stuff, and it looks like he wanted to cover for lots of data types - from bool to long double (see NiftiImageData.h)
at any rate, I doubt we would ever need any dataType other than float
to remind you: dataType cannot be complex - complex images are treated as pairs of real ones
There was a problem hiding this comment.
Checking copy_nifti_image it contains
SIRF/src/Registration/cReg/NiftiImageData.cpp
Line 683 in 60bda8a
therefore, it needs to have the same data-types, see these lines are correct.
Note that it can be simplified to
auto nii_ptr = dynamic_cast<const NiftiImageData<dataType> * const >(&to_copy)
There was a problem hiding this comment.
therefore, it needs to have the same data-types
so far as I can see, this is to be taken care of by nifti_copy_nim_info
Note that it can be simplified
thanks, adopted
| else { | ||
| this->_nifti_image = NiftiImageData<float>::create_from_geom_info(*to_copy.get_geom_info_sptr()); | ||
| // Always float | ||
| this->set_up_data(NIFTI_TYPE_FLOAT32); |
There was a problem hiding this comment.
this looks incorrect to me. *this should always have dataType, otherwise templating this makes no sense. So we need to set-up with the corresponding NIFTI.... (Easiest to do by having a helper class with template specialisations for supported dataTypes).
There was a problem hiding this comment.
I believe this is only needed if we decide to use nifti data types other than DT_FLOAT32.
| @@ -102,7 +139,11 @@ NiftiImageData<dataType>& NiftiImageData<dataType>::operator=(const ImageData& t | |||
| // Always float | |||
| this->set_up_data(NIFTI_TYPE_FLOAT32); | |||
| auto &it_src = to_copy.begin(); | ||
| auto &it_dst = this->begin(); | ||
| for (; it_src != to_copy.end(); ++it_src, ++it_dst) | ||
| *it_dst = (*it_src).complex_float().real(); |
There was a problem hiding this comment.
this is truncating complex to float. 2 remarks:
- This would be amazingly confusing if
dataTypeis complex. I know you're saying this can currently not happen, but do we guarantee it anywhere? - If
dataTypeisfloatordouble, then this in my opinion shouldn't be in an assignment operator.
There was a problem hiding this comment.
No, dataType cannot be complex - see 12 data types dealt with by change_datatype.
As the destination can only store float data while the source has complex_float_t data, some kind of truncation (either abs or real) is unavoidable I am afraid.
Changes in this pull request
This PR takes over from PR #1195, which was left unattended for a long time, resulting in weird conflicts with master.
Testing performed
All tests run by
ctestsucceeded.Related issues
Note
Breaks backward compatibility
Checklist before requesting a review
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following: