Which should now be the `=delete` implementation, which should give a useful error message if the method is used.
The problem with the superclass implementation is ITK does not use multiple inheritance. This would make it quite challenging to integrate it into our current hierarchy…
I vote we replace the macro with an explicit `=delete` implementations.
---
### Author:  [@matt.mccormick](https://discourse.itk.org/u/matt.mccormick)
#### Post date: [February 6, 2018, 2:31pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/3 "2018-02-06T14:31:20Z")
> [@blowekamp](#):
>
> I vote we replace the macro with an explicit =delete implementations.
+1
---
### Author:  [@fbudin](https://discourse.itk.org/u/fbudin)
#### Post date: [February 6, 2018, 2:33pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/4 "2018-02-06T14:33:43Z")
Is it really clearer for someone reading the source code to see `=delete` instead of `ITK_DISALLOW_COPY_AND_ASSIGN`? For an experienced C++ developer, maybe, but from a new developer, seeing the macro that has a pretty explicit name is failry clear. The only advantage I see in replacing the macro is to grep `=delete` in the code.
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [February 6, 2018, 3:08pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/5 "2018-02-06T15:08:36Z")
In general, I’m not a fan of using something like **boost::noncopyable** as a _base class_ in those cases where it would cause _multiple inheritance_. Multiple inheritance is an interesting and powerful tool in C++, but I’m reluctant to use it, as it tends to make things overcomplicated. Think about the diamond problem: [https://en.wikipedia.org/wiki/Multiple\_inheritance](https://en.wikipedia.org/wiki/Multiple_inheritance)
In general, I’m not a fan of preprocessor macro’s either, but still I find the
**ITK\_DISALLOW\_COPY\_AND\_ASSIGN(x)** macro quite clear and convenient. 😀
My 2 cents
---
### Author:  [@blowekamp](https://discourse.itk.org/u/blowekamp)
#### Post date: [February 7, 2018, 1:32pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/6 "2018-02-07T13:32:22Z")
If we consider that there is going to be additionally frequent use of:
```auto
MyClass() = default;
~MyClass() = default;
```
Then the `=delete` will look just fine next to these. As opposed to some unknown macro.
```auto
protected:
MyClass() = default;
~MyClass() = default;
private:
MyClass(MyClass) = delete;
MyClass &operator(MyClass) = delete;
```
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [February 8, 2018, 2:07pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/7 "2018-02-08T14:07:00Z")
Of course, I wouldn’t stop you from removing ITK\_DISALLOW\_COPY\_AND\_ASSIGN(x). I’m just saying that I personally find this macro quite clear and convenient 😀
When you’re declare the copy constructor and the copy assignment operator as _deleted functions_, I would suggest you to do it the “canonical” way:
```
public:
MyClass(const MyClass&) = delete;
MyClass &operator=(const MyClass&) = delete;
```
So I’d suggest declaring their parameter as _const reference_, and to declare them _public_, following Scott Meyers Effective Modern C++ item 11, “Prefer deleted functions to private undefined ones”.
My 2 cents again!
---
### Author:  [@blowekamp](https://discourse.itk.org/u/blowekamp)
#### Post date: [February 8, 2018, 2:16pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/8 "2018-02-08T14:16:17Z")
My recent implementations of `operator=` have been without the reference in the argument due to considerations for “rvalue reference” and a move constructor. What was the “canonical” implementations in C++98, is clearly not longer in C++11.
Also It is not immediately clear to my why the `public` declaration is preferred? I believe currently they are private and deleted.
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [February 8, 2018, 4:53pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/9 "2018-02-08T16:53:36Z")
> [@blowekamp](#):
>
> My recent implementations of operator= have been without the reference in the argument due to considerations for “rvalue reference” and a move constructor. What was the “canonical” implementations in C++98, is clearly not longer in C++11.
Can you please elaborate? In general, I’m reluctant to do pass-by-value for objects that are expensive to copy. In some cases, the compiler may eliminate the actual copying, but it cannot do so in _all_ cases.
> [@blowekamp](#):
>
> Also It is not immediately clear to my why the public declaration is preferred? I believe currently they are private and deleted.
The compiler should produce a more specific error message, when a _public_ deleted function is called, rather than when a _private_ deleted function is called.
VS2015 produces the following error on an attempt to call a _public_ deleted function:
```
error C2280: 'A &A::operator =(const A &)': attempting to reference a deleted function
```
See [Public deleted function, C++ (vc++) - rextester](http://rextester.com/EIMWF88756)
While it produces the following error on an attempt to call a _private_ deleted function:
```
error C2248: 'A::operator =': cannot access private member declared in class 'A'
```
See [Private deleted function, C++ (vc++) - rextester](http://rextester.com/MCEK16785)
Does that convince you, or not?
---
### Author:  [@blowekamp](https://discourse.itk.org/u/blowekamp)
#### Post date: [February 8, 2018, 7:31pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/10 "2018-02-08T19:31:35Z")
The copy constructor written without a reference was non-sense.
But the now current modern C++ copy-swap(and move) idiom utilized the `MyClass &operator(MyClass)"`signature. Were the argument can be either copy constructor or move constructed. This is well described on [stack overflow](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). Additionally, when the object support move, and a copy is going to need to be made the copy [can be passed by value](http://review.source.kitware.com/#/c/23151/). This looks odd to me too.
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [February 8, 2018, 10:04pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/11 "2018-02-08T22:04:33Z")
Copy-and-swap is a great concept, but it does not _always_ yield the best performance, for all relevant use cases. Suppose you have two vectors (std::vector or vnl\_vector) v1 and v2, both having 1000 elements. How would you implement the copy-assignment v2 = v1? Copy-and-swap would do:
```
{
auto temp = v1; // Copy. Involves dynamic allocation of 1000 elements.
swap(temp, v2);
} // Destruction of temp, involves deallocation of memory of 1000 elements.
```
The following would be much faster, because it would not need to do allocation + deallocation of temporary memory.
```
std::copy(v1.begin(), v1.end(), v2.begin());
```
In cases like this, it might be worth overloading operator= for lvalue and rvalue references:
```
MyVector& operator=(const MyVector&); // May reuse and overwrite existing memory from 'this'
MyVector& operator=(MyVector&&) noexcept; // As fast as a 'swap'.
```
Anyway, [http://review.source.kitware.com/#/c/23151/](http://review.source.kitware.com/#/c/23151/) (_Allow compiler to choose best way to construct a copy_) looks fine to me 🙂 These are indeed cases in which pass-by-value is superior!
My 2 cents.
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [March 21, 2018, 2:13pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/12 "2018-03-21T14:13:03Z")
My proposed patch, [STYLE: Added DISALLOW\_COPY\_AND\_ASSIGN(GaussianDerivativeImageFunction), etc.](http://review.source.kitware.com/#/c/23269/1) adds an _ITK\_DISALLOW\_COPY\_AND\_ASSIGN_ macro call to a _ **public** _ section of a class.
@matt.mccormick initially rejected this patch, because the ITK Style Guide suggests to place this macro call in a _ **private** _ section of the class:
>
This very much made sense at the time the guideline was written, as in old C++03 code, it was in general recommended to disallow copying by declaring copy constructor and assignment operator _private_. However, when copying is disallowed by using modern C++11 _‘= delete’_ syntax (as the macro does by now, [commit c1aba33](https://github.com/Kitware/ITK/commit/c1aba333904c19fda9da878da5c007f47ca05018)), it appears common practice to declare those deleted member functions _public_! As [Scott Meyers](https://www.aristeia.com/) wrote in [Effective Modern C++](http://shop.oreilly.com/product/0636920033707.do), Item 11 (page 75-76):
> By convention, deleted functions are declared _public_, not _private_. There’s a reason for that. When client code tries to use a member function, C++ checks accessibility before deleted status. When client code tries to use a deleted _private_ function, some compilers complain only about the function being _private_, even though the function’s accessibility doesn’t really affect whether it can be used. It’s worth bearing in mind when revising legacy code to replace private-and-not-defined member functions by deleted ones, because making the new functions _public_ will generally result in better error messages.
Classes from the Standard C++ Library (_std_) also follow this conversion, as can be seen at [http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4727.pdf](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4727.pdf)
Do you agree that it’s OK for ITK code to follow this convention of declaring deleted functions **_public_**? At least, I hope my patch can now be found acceptable: [http://review.source.kitware.com/#/c/23269](http://review.source.kitware.com/#/c/23269)
---
### Author:  [@blowekamp](https://discourse.itk.org/u/blowekamp)
#### Post date: [March 21, 2018, 2:25pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/13 "2018-03-21T14:25:18Z")
As Matt pointed out, ITK already has an established convention on how and where to delete these methods. While if we were to make the design choice today we would likely follow your recommendation, we already have a convention well established and uniform across the toolkit. So for the sake of consistency we should go along with the establish convention.
Now if you are so motivated to update all of ITK to your postposed NEW convention, that is a different ( and welcomed ) conversation.
---
### Author:  [@dzenanz](https://discourse.itk.org/u/dzenanz)
#### Post date: [March 21, 2018, 3:03pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/14 "2018-03-21T15:03:13Z")
@Niels_Dekker Updating just one class to have copy constructor and assignment operator publicly deleted does not make much sense from consistency standpoint. If you are willing to update it throughout the toolkit that would be awesome!
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [March 25, 2018, 7:39pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/15 "2018-03-25T19:39:38Z")
@dzenanz, @blowekamp, @matt.mccormick OK, here it is:
_COMP: Moved ITK\_DISALLOW\_COPY\_AND\_ASSIGN calls to public section_
[http://review.source.kitware.com/#/c/23289/](http://review.source.kitware.com/#/c/23289/)
Please review before it gets any merge conflicts! 😀 (Rebasing and recommitting the patch seems quite time consuming for this large number of files.)
---
### Author:  [@dzenanz](https://discourse.itk.org/u/dzenanz)
#### Post date: [March 26, 2018, 1:10pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/16 "2018-03-26T13:10:44Z")
For the record, patch has been swiftly reviewed and merged. Thanks for the contribution Niels!
---
### Author:  [@dzenanz](https://discourse.itk.org/u/dzenanz)
#### Post date: [March 26, 2018, 1:22pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/17 "2018-03-26T13:22:35Z")
This might be a lot to ask for, but @Niels_Dekker can you apply that script to remote modules?
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [March 26, 2018, 2:07pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/18 "2018-03-26T14:07:52Z")
> [@dzenanz](#):
>
> @Niels_Dekker can you apply that script to remote modules?
Hi @dzenanz,
Where are those remote modules located?
The script is now at:
>
It’s pretty much “Standard C++” (no platform-specific dependencies), but it depends on \ being available (to iterate over the source tree). It builds out of the box on Visual C++ 2017. (No CMakeLists necessary!) Maybe you can give it a try?
It expects the directory path of the source tree as command-line argument, and modifies the files _in-place_ so make sure you have a back-up!
---
### Author:  [@jhlegarreta](https://discourse.itk.org/u/jhlegarreta)
#### Post date: [March 26, 2018, 2:19pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/19 "2018-03-26T14:19:13Z")
First of all, hats off to @Niels_Dekker and the script he wrote. Impressed by it.
As for the remotes, each remote module dwells in a repository of its own. Many are under the [InsightSoftwareConsortium](https://github.com/InsightSoftwareConsortium) organization/group in Github; others may be under [KitwareMedical](https://github.com/KitwareMedical); and others are in some other GitHub repositories.
One should examine each `.cmake` file under the [remote modules](https://github.com/InsightSoftwareConsortium/ITK/tree/master/Modules/Remote) to see where they dwell.
@hjmjohnson has made a huge effort to adapt these to C++11, so may he used some `CMake` script to checkout, create branches, etc. all at a time, and then we could apply Niels’ script could be applied? Hans?
---
### Author:  [@Niels\_Dekker](https://discourse.itk.org/u/Niels_Dekker)
#### Post date: [March 26, 2018, 3:26pm UTC](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648/20 "2018-03-26T15:26:57Z")
My script [https://gist.github.com/N-Dekker/a46cd003f19a84c6d285c2487fc5e13d](https://gist.github.com/N-Dekker/a46cd003f19a84c6d285c2487fc5e13d) found 1343 DISALLOW\_COPY macro calls in the ITK source tree, of which it properly moved 1300 to the public section. That’s a success rate of almost 97% 🤩
I’m still trying to manually fix the 43 DISALLOW\_COPY macro calls that were not handled by the script. 7 of them I did here already: [http://review.source.kitware.com/#/c/23292/1](http://review.source.kitware.com/#/c/23292/1)
Some of the header files that the script did not handle are formatted in different way from what I expected. E.g., by having the _class_ keyword and the _class name_ on different line of code. Or by having an export macro that I did not see before, like ITK\_FORCE\_EXPORT\_MACRO(ITKVideoCore), or ITKIOImageBase\_HIDDEN… Work in progress!
[Next page](https://discourse.itk.org/t/itk-disallow-copy-and-assign/648.md?page=2)