Page MenuHomePhabricator

Rethink property assignment
Closed, ResolvedPublic

Description

After reading http://icu-project.org/docs/papers/cpp_report/the_assignment_operator_revisited.html I think the current implementation of property assignment operators is still not completely correct:

"So there you go. A truly foolproof method of handling polymorphism in assignment operators involves declaring both a virtual and a non-virtual assignment operator in every class (except the root class of each inheritance hierarchy), with the non-virtual calling the virtual and the virtual asserting that both objects involved are the same class. Any time a calling function couldn’t guarantee the invariant would hold, it would have to avoid using the assignment operator and manually delete the object referenced by the target variable and new up a new one of the proper type.

Beautiful, huh?"

For most of our property classes, both operators are trivial, since the subclasses do not add new members. So maybe this does not affect us or we could limit the way properties can be assigned in some way. Or we can check some more literature for solutions to this kind of problem.

Related Objects

StatusAssignedTask
ResolvedNone
ResolvedNone

Event Timeline

The proposed changes are visible in the branch

bug-8889-fix-property-comparison-and-assignment

[ead996]: Merge branch 'bug-8889-fix-property-comparison-and-assignment'

  • bug-

Merged commits:

2011-10-27 20:49:29 Sascha Zelzer [f0c024]
Removed unnecessary "typename".


2011-10-18 15:08:55 Sascha Zelzer [e58280]
Fixed warnings.


2011-10-18 15:08:45 Sascha Zelzer [52bd6c]
Fixed property assignment issues.

mitk::BaseProperty now uses the non-virtual interface (NVI) idiom
for polymorphic assignment. The assignment operator and copy
constructor of subclasses of mitk::BaseProperty should be disabled
and bool Assign(const BaseProperty&) should be implemented instead
(either private or protected).

Assignable(const BaseProperty&) has been removed and
mitk::PropertyList::SetProperty(...) has been simplified.

New unit tests for polymorphic comparison and assignment of properties.


2011-10-17 18:02:47 Sascha Zelzer [39f6d7]
Use the NVI idiom for comparing property objects.

This avoids unnecessary virtual function calls and dynamic casts.

[c7096e]: Merge branch 'bug-8889-fix-property-comparison-and-assignment'

  • bug-

Merged commits:

2011-10-29 11:40:39 Sascha Zelzer [2346a3]
COMP: Use correct numerical suffix

[7ee8c3]: Merge branch 'bug-8889-fix-property-comparison-and-assignment'

  • bug-

Merged commits:

2011-10-31 11:51:50 Sascha Zelzer [d8aa70]
Make sure tests use different mitk::LookupTable objects

files in MBI ResectionProposalDescriptorProperty needs to be adapted as well

same issue with property: VesselTreeLookupTableProperty

Please report issues with MBI code in bugs for the MITK-mbi product. See T9950.