ENH: Add operator==, operator!= to MetaDataObject and MetaDataDictionary

[N-Dekker]

Follow-up to "ENH: Add operator== and operator!= to itk::Image"
pull request #2188
commit 7f3337d

[dzenanz]

Looks good to me, aside from compile warnings.

[blowekamp]

Why is this typeid comparison done over using a dynamic_cast?

Beware there are monsters here. There have been issues related to shared libraries, linkage and duplicate symbols ( typeids ) not being equal on some operating symbols (OS X), which cause these types of comparisons or dynamic_cast to fail which the object refer to the same class/lines of code but the type information is from difference complications units. I hope we have addressed they will not reoccur. However, giving this history I need to and some checks to help if this does reoccur.

Please break up this compound statement. When the typeid or dynamic_cast fails we should add an additional check to see if the objects are from the same class. The type_info's name method may provide this check. So add if the first comparison fails: if ( typeid(metaDataObject).name() == typeid(Self).name() ) itkWarningMacro("..."). This will be of great use if the linkage issue reoccurs.

[N-Dekker]

@blowekamp Thanks for your feedback. I used typeid, to state explicitly that two meta-data-objects only compare equal when their types are equal. I think the intension of typeid(metaDataObject) == typeid(Self) is most clear.

However, if you prefer dynamic_cast, that would be fine to me as well. I could certainly make it:

const auto metaDataObjectAsSelf = dynamic_cast<const Self *>(&metaDataObject);
return (metaDataObjectAsSelf != nullptr) && (*this == *metaDataObjectAsSelf);

Would you like that better?

Regarding those possible issues related to shared libraries, linkage and duplicate symbols, does it help that MetaDataObject<T> is explicitly instantiated for a specific set of types? In https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2rc01/Modules/Core/Common/src/itkMetaDataObject.cxx

I would rather not put a itkWarningMacro in there, as I think by definitions, operator== should not have any side effects.

[blowekamp]

const auto metaDataObjectAsSelf = dynamic_cast<const Self *>(&metaDataObject);
return (metaDataObjectAsSelf != nullptr) && (*this == *metaDataObjectAsSelf);

Would you like that better?
@N-Dekker Yes. It is more consistent with ITK, and less likely to be detected as improper use of static_cast.

I would rather not put a itkWarningMacro in there, as I think by definitions, operator== should not have any side effects.

It will improve robustness for a problematic situation that likely can still occur in some configuration of ITK. It is important that there are checks in ITK to ensure the code is robust and errouneous states and able to be tracked down. It will not have side effect ( display warning ) unless the problem is encountered, and it will make the issue be able to tracked down.

[N-Dekker]

My idea with this PR is that (metaDataObjectBase1 == metaDataObjectBase2) would return false, and (metaDataObjectBase1 != metaDataObjectBase2) would return true, when they refer to objects whose value type is different. The == and != comparisons themselves would then still be valid. That's why I think a warning or error message is not necessary. OK?

[N-Dekker]

This pull request is just a nice-to-have to me. It would be nice to be able to compare meta-data this way. But it isn't something I need right away.

[dzenanz]

Well, now is a good time to merge this. Brad?