[Insight-developers] Questions about operator= in ITK

[Bradley Lowekamp]

Kent,

1 ) I agree it seem wrong to me. If the method is returning void, then no one is using that value, returning the correct value shouldn't be a big deal. Since the assignment operator is generally needs to be done on the real object not a superclass pointer, it should not be a big deal for derived classed.

2) This looks correct to me. Notice the operator= should not be virtual, and while both the derived and parent class define them as "self & operator=(const Self & other)", the "Self type is different. I believe this is needed.

3) It the object need to allocate memory or do real work, I think is should check. With most of ITK using smart pointers I don't think it's a big deal, or likely to cause an error. So it comes down to performance. Yes the smart pointers do use atomic increment and decrement, which can be expensive in certain multi-threaded situations. I don't think it would be a measurable performance gain.

My personal favorite way to implement the assignment operator is using the copy constructor, followed by a swap.

On Mar 28, 2013, at 10:33 AM, "Williams, Norman K" <norman-k-williams at uiowa.edu> wrote:

> In an unrelated investigation about C++ operator=, I came upon a
> recommendation to always check if an object is being assigned to itself.
> 
> At best, you can avoid potentially expensive code that does nothing. At
> worst, you can get tangled up in a maze of problematic object creation and
> deletion.
> 
> So I began an audit of operator= in ITK, using grep-find in Emacs with
> this command:
> 
> find . -type f -exec grep -nH -e 'operator=[^=]' {} /dev/null \; | grep
> -iv 'not impl' | grep -v ThirdParty | grep -v 'Superclass::operator=' |
> grep -v 'purposely not'
> 
> In other words, ignore the cases where it is declared but not implemented,
> which is the proper behavior of ITK object types.
> 
> I've discovered some curiosities:
> 
> 1. operator= returning void -- this seems like it's completely wrong, even
> if the result is never used in practice.
> 
> 2. Explicitly invoking Superclass::operator=
>  Self & operator=(const Self & other)
>  {
>    Superclass::operator=(other);
>    return *this;  }
> 
> 3. Many cases of not checking for self-assignment.
> 
> I'm working on a patch to try and address these issues, but it raises some
> questions.
> 
> Is case #2 as redundant as it appears? Should those 'defer to superclass'
> assignment methods remain?
> 
> In the first case, it amounts to a change in the public API. Is this going
> to break anything?
> 
> As regards case #3, is there any reason NOT to add the self-assignment
> check?
> 
> 
> --
> Kent Williams norman-k-williams at uiowa.edu
> 
> 
> 
> 
> 
> 
> ________________________________
> Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged.  If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited.  Please reply to the sender that you have received the message in error, then delete it.  Thank you.
> ________________________________
> _______________________________________________
> Powered by www.kitware.com
> 
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
> 
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.php
> 
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
> 
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers