Should we implement a mechanism similar to boost’s noncopyable and put it somewhere in the filter class hierarchy?
I have been using this in SimpleITK:
Traditionally ITK has been very explicit and verbose with things. Previously we did explicitly declared but did not define these methods; the methods were “purposely not implemented”:
This resulted in a non-intuitive linking error.
Now we have a macro:
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.
+1
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.
3 Likes
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
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
1 Like
If we consider that there is going to be additionally frequent use of:
MyClass() = default;
~MyClass() = default;
Then the =delete will look just fine next to these. As opposed to some unknown macro.
protected:
MyClass() = default;
~MyClass() = default;
private:
MyClass(MyClass) = delete;
MyClass &operator(MyClass) = delete;
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!
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.
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.
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
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
Does that convince you, or not?
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. Additionally, when the object support move, and a copy is going to need to be made the copy can be passed by value. This looks odd to me too.
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/ (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.
1 Like
My proposed patch, STYLE: Added DISALLOW_COPY_AND_ASSIGN(GaussianDerivativeImageFunction), etc. 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), it appears common practice to declare those deleted member functions public! As Scott Meyers wrote in Effective Modern C++, 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
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
2 Likes
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.
1 Like
@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!
2 Likes
@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/
Please review before it gets any merge conflicts! 
(Rebasing and recommitting the patch seems quite time consuming for this large number of files.)
2 Likes
For the record, patch has been swiftly reviewed and merged. Thanks for the contribution Niels!
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 <experimental/filesystem> 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!
1 Like
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 organization/group in Github; others may be under KitwareMedical; and others are in some other GitHub repositories.
One should examine each .cmake file under the remote modules 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?
1 Like
My script 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
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!
1 Like