[RFC-24] Updated public draft

[Umberto Nicoletti]

Gee, this thread is getting really hard to follow even for me!

On 3/1/07, Tamas Szekeres <szekerest at gmail.com> wrote:
> 2007/2/25, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> >
> > I have checked and it seems that we can add a check at the beginning
> > of all functions where a layer uncorrectly dereferences the map
> > pointer without checking. The check will be  like this:
> >
> > if (layer->map == NULL) {
> >     // should we raise MS_CHILDERR instead?
> >     msSetError(MS_MEMERR, "map is null", "");
> >     return MS_FAILURE;
> > }
> >
>
> Umberto,
>
> I've spent some time tracking down the relationship between the
> various mapscript classes and even an UML diagram have been created
> for this purpose.
> Currently the lifetime of many of the objects are controlled by other objects.
> Some of the objects can be considered as 'root object' and can control
> the destruction of its own memory safely. However many of the classes
> are considered as descendant of the root and should eventually be
> destroyed by the root object.
> Furthermore some of the classes (like hashtableObj, colorObj,
> labelObj, rectObj) may belong to various class types.
> Some of the classes may appear as root and can also appear as
> descendant class (like shapeObj, pointObj, projectionObj, rectObj)
>
> In this regard the current memory management model cannot be altered
> safely and would require large amount of work to implement. Every
> nested class members should be rewritten as dynamically allocated
> members in this case.
> In addition the issue how the back-references are handled denotes
> obviously that we should not bother with this kind of work anymore. In
> addition it's not the best way to treat
> the problem as raising an error when the back reference is null for a
> particular
> situation, because the user cannot see obviously why that error have
> occurred and how to protect against it.

I've added a patch to the tracker bug
http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=2032
that implements the check for layer->map and it seems to throw up a
reasonable error.
Maybe are we underestimating our users?

> When a particular class is considered as a potential child class, it's
> definitely unsafe to support the separation of this class, and change
> the ownership of the memory at run time.
>
> I would favour making some stripping on the interface to properly
> reflect how the objects are managed internally.
> Some of the objects should not be externally creatable (layerObj,
> classObj for example) and some members should not be writable etc.
>
> According to the issues mentioned above to properly implement a
> reference counting approach at the C side some kind of
> object memory context management should be introduced. All of the
> dependent object should be assigned to the same context at run time.
> Though the framework of this concept could be created easily, the
> proper run time assignment of these contexts would require large
> amount of coding as I can see and would destabilize the current
> implementation.
>
> >
> > If we correct this do we still need to keep a parent reference in the
> > child mapscript object?
> >
>
> Currenly I would rather implement this parent reference approach at the target
> language side in a generalized fashion. Recall that a particular type
> may belong to various other types so this parent reference cannot be a
> strongly typed reference to a specific mapscript class type.
> I've attached my solution specifically for the C# interface but it
> might be easily rewritten to java as well.
> This approach appears to operate for the GDAL C# fairly well.
> Here's the C# example was tested, which was systematically failed
> without using this patch.
>
>    mapObj m_obj = new mapObj(args[0]);
>
>    Console.WriteLine ("# Map layers " + m_obj.numlayers + "; Map name
> = " + m_obj.name);
>
>    for (int i=0; i<m_obj.numlayers; i++)
>
>    {
>      Console.WriteLine("Layer [" + i + "] name: " + m_obj.getLayer(i).name);
>
>    }
>    layerObj layer1 = m_obj.getLayer(1);
>    layerObj layer2 = m_obj.getLayer(2);
>    layerObj layer3 = m_obj.getLayer(3);
>    layerObj layer4 = m_obj.getLayer(1);
>    layerObj layer5 = m_obj.getLayer(2);
>    layerObj layer6 = m_obj.getLayer(3);
>    GC.Collect();
>    classObj classo = layer5.getClass(0);
>

Tamas, doesn't this look like my previous proposal (keep references in
the mapscript wrapper classes and don't touch the C layer)?

>
>
> We should continue to deal with rewriting the arrays of structs to
> arrays of pointers. Implementing Add/Remove on the collections of
> classes cannot be done properly with the current solution. But this
> problem can be handled separately.
>

Agreed.

A general consideration, please don't take it as rudeness I am just
getting straight to the core of the issue at hand:

at this point I am not quite sure about the road the want to take. My
first proposal was in my opinion easy doable, but it required a
separate implementation for every mapscript.
Because of this I was asked for something more portable across
mapscripts and maybe even php so I came up with the second release of
RFC-24.
And now, again, it seems like we are going back to a slightly
different version of the first rfc24.

Now, I want to clarify that I am doing this in my spare time and with
little to no funding so I can tolerate one or two of these U-turns,
but I can't simply afford to go on forever discussing and modifying
documents. Also please understand that I am NOT saying this discussion
is useless or annoying, I highly value the exchange of ideas with
fellow programmers (especially if talented like most of you), as long
as they become real sooner or after.

So my proposal is that I attach patches to the tracker bug and rather
discuss on those than on this rfc. This would also have the benefit
that once we vote and if the rfc gets a positive feedback the
implementation is already adavanced to the point that you can already
see something working.

Umberto

>
> Best regards,
>
> Tamas
>
>