[vtk-developers] Maybe ABSTRACT isn't needed

[Marcus D. Hanwell]

On Wed, Oct 24, 2012 at 12:19 PM, David Gobbi <david.gobbi at gmail.com> wrote:
> On Wed, Oct 24, 2012 at 9:20 AM, Marcus D. Hanwell
> <marcus.hanwell at kitware.com> wrote:
>
>> On Wed, Oct 24, 2012 at 12:43 AM, David Gobbi <david.gobbi at gmail.com> wrote:
>>> On Tue, Oct 23, 2012 at 11:40 AM, Brad King <brad.king at kitware.com> wrote:
>>>>
>>>> IMO it is still worth dropping the wrappers' need for ABSTRACT
>>>> if possible.
>>>
>>> I've pushed a wrapper patch to gerrit.
>>>
>>> I think the instantiators could be similarly fixed, if we added a
>>> new wrapper tool to build the instantiator source files for each
>>> module (rather than have the instantiator source created by
>>> vtkMakeInstantiator.cmake like it is now).
>>>
>> Did you take into account the abstract base classes that have New
>> methods that can return NULL? This was carried over from the VTK
>> factory classes where they would return NULL if the correct set of
>> defines was not present. Any class using
>> vtkAbstractObjectFactoryNewMacro can and does return NULL if an
>> override was not specified. This is so that the interface class can be
>> instantiated, and the correct derived form returned at runtime.
>
> The fact that the wrappers crash when New() returns NULL is indeed a
> problem.  But marking factory classes as ABSTRACT was never the
> correct solution.  As far as the wrappers are concerned, factory
> classes like vtkRenderWindow have always been treated as concrete,
> because the wrappers have always used New(), rather than the class
> constructor, to create new instances.
>
> The confusion over what the ABSTRACT property means is one of the
> reasons why I would like to remove it.  Many C++ programmers will
> automatically assume that a class with virtual methods should be
> marked ABSTRACT, but that is not the case if the class is a factory
> class with a New() method.

That sounds good, I just wanted to ensure that people were aware of
this behavior (and the fact that they were never marked abstract but
do return NULL from a New()).
>
>> Several of those classes are pure virtual, and so preserving the
>> existing behavior seemed like the only reasonable course of action. If
>> New can never return NULL (I didn't see that anywhere, and found
>> several places where this was not the case) then the abstract object
>> factory new macro would need to be modified, it could call abort or
>> similar if that was desirable? I didn't think too deeply about this,
>> and preserved the behavior that was already present although it is
>> easier to see this present in applications now.
>
> An abort is rather harsh.  A NULL returned by a New() method is
> probably best handled by an exception in the wrapper language (not a
> C++ exception, but a wrapper-language exception).
>
I was hoping to avoid anything too harsh, this sounds reasonable to me
- NULL is easy to detect. Just to be clear I support removing the
ABSTRACT keyword, but didn't want this to cause major issues in the
factory classes.

Thanks for working on this.

Marcus