That makes sense, thanks.
What about the possibility of New() forwarding to constructors like std::make_shared()?
New first checks whether any of the registered factories provides a constructor for the class, and if not uses the constructor directly. Here is its implementation.
1 Like
Hi @dzenanz, I donāt think I was clear about what I meant. Iāve done some reading so I can be clearer. Ultimately, I would like to be able to write code like:
auto my_filter = itk::AFilterType::New(argument1, argument2);
instead of:
auto my_filter = itk::AFilterType::New();
my_filter->SetArgument1(argument1);
my_filter->SetArgument2(argument2);
I personally find the 1st version clearer and more idiomatic C++ (as it follows the RAII style). I donāt for a minute propose getting rid of the 2nd version though, only adding this as an option.
You can do this with std::make_shared, e.g.:
auto a_pointer = std::make_shared<A_Type>(argument1, argument2);
This is accomplished by make_shared taking a parameter pack as one of its template arguments, and then forwarding that to the constructor. I think it would be relatively straightforward to write a macro that could do this for ITK in the non-factory case. How this would work with factories, I donāt know (perhaps it wouldnāt need to?).
I think this can be accomplished, and easily too. Add a static method New with two arguments, and in implementation do:
auto my_filter = itk::AFilterType::New();
my_filter->SetArgument1(argument1);
my_filter->SetArgument2(argument2);
return myFilter;
A constructor with two parameters approach might be similarly possible too.
The ITK Python wrapping already supports this behavior. If possible to have similar behavior, it would be nice to have the same API.
Your example implies that āSetArgument1ā and āargument1ā are similarity named and thus obviously redundant. I think a better example would be:
auto my_filter = itk::AFilterType::New();
my_filter->SetArgument1(variableA);
my_filter->SetArgument2(variableB);
vs
auto my_filter = itk::AFilterType::New(variableA, variableB);
Hereās its more clear that the self-documenting āSetArgument1ā is lost.
If C++ had named parameters, like Obj-C or Swift, then you could do something like:
auto my_filter = itk::AFilterType::New(argument1 : variableA, argument2 : variableB);
But without named parameters, a constructor that just takes a bunch of arguments, especially if they themselves are declared āautoā, I think could really hurt readability of the code.
Cheers,
1 Like
The ITK Python works nicely because it utilizes named arguments which clearly states which argument is which.
In SimpleITK, we have procedural methods and Execute methods which takes a list of arguments. It a tricky to maintain the API and keep the order of the arguments. Additionally, there are permutations when an argument can have multiple types. For example, scalar and array. I donāt think this can be properly maintained in ITK manually.
We are able to do it in SimpleITK because the SimpleITK interface is automatically generated from a JSON file which specifies each IVAR/parameter once. Then the IVARs Set/Get methods, procedural interface, and execute interface are all generated consistently. Even options for multiple types of arguments are taken into consideration and the permutations are automatically generated. This can not be done manually.
@seanm Yes - this was exactly the discussion I thought would occur. Itās a matter of personal preference which to go for. I would prefer the RAII type initialisation because I often forget to add all the required ->SetArgument() calls and then end up with objects in invalid states which only gets picked up at run-time. RAII is more likely (but not foolproof) to catch problems at compile-time. And yes, named arguments could be the best of both-worlds (although Object-C ends up horribly verbose in my opinion).
@matt.mccormick / @dzenanz I think the best way to implement this would be an additional macro, e.g. itkForwardNewMacro that declares the necessary template function? I will try to come up with a patch to show what I have in mind (but my todo list is very long 
)
I often forget to add all the required ->SetArgument() calls and then end up with objects in invalid states
If Iām not mistaken some compilers are able to warn about uninitialized variables, and dynamic analysis tools also report them.
In any case, what the Coding Style Guide chapter in Appendix 2 of the ITK Software Guide says (in section B.10):
All member variables must be initialized in the class constructor. For such purpose, initialization lists are preferred
True that you will find some cases where this has been overlooked. But weād need to try to fix them whenever we find the,
Hello,
Apologies for taking 6 months to get round to this, but the following macro does what I want:
#define itkForwardNewMacro(x) \
template<class... Args> \
static Pointer New(Args &&... args) \
{ \
Pointer smartPtr = ::itk::ObjectFactory< x >::Create(); \
if ( smartPtr == nullptr ) \
{ \
smartPtr = new x(std::forward<Args>(args)...); \
} \
smartPtr->UnRegister(); \
return smartPtr; \
}
@jhlegarreta Thereās different levels of uninitialized. For instance, if I forget to set the second input to an AddImageFilter, ITK will not crash, but it will correctly throw an exception telling me that the input hasnāt been set. Iād prefer to pick that up at compile time by not having the correct number of arguments to my constructor, which the current architecture prevents. The closer I can get to RAII the better, but I am aware I may be in the minority with that opinion.
1 Like
Glad to see that you found the macro useful.
I guess it was a design option to disallow/not use such constructors. May be some of the more experienced folks can elaborate more on the reasons.
For the example of AddImageFilter and similar existing implementations, if we want to pass the inputs in the construction of the object via New(Args ... args), the number of required inputs in existing implementations is defined in constructors, with SetNumberOfRequiredInputs, so the implementation could like something like (untested):
if ( smartPtr == nullptr )
{
unsigned int index = 0;
smartPtr = new x();
for ( auto input : {args...} )
{
smartPtr->SetInput( index, input );
++index
}