Bug 1803, Upcoming breaking changes for 4.10.0-beta1

Umberto Nicoletti umberto.nicoletti at GMAIL.COM
Wed Aug 23 08:50:54 EDT 2006 

Tamas,

On 8/23/06, Tamas Szekeres <szekerest at gmail.com> wrote:
> I would discourage this kind of solution due to the following reasons:
>
> 1. The proposed change results in memory leak since the original
> structure is not destroyed properly. As a workaround  freeWeb() might
> be called at the beginnings.

if you look closely at the macros in mapcopy.h you'll see that memory
is correctly deallocated. There should be no need to deallocate the
original structure as we only copy the members.

> 2. It's the developer's responsibility not to impulse using such a
> solution that results in extra memory deallocations and allocations
> which are completely avoidable by using the
> interface correctly. If we allow many ways to achieve the same results
> no one will have enough internal information to make the decision and
> choose the most convenient way he wants. For example i am not totally
> sure whether folks are fully aware of the benefits of
> using 'new layerObj(map)' over insertLayer() when adding a layer to
> the map. I have seen such an implementation where both of the
> approaches were used at the same time.
>

I think you mistake correctness for performance. Your solution is
probably more performant, but both are correct.

> 3. At C#'s perspective this addition will also break the backward
> compatibility so i cannot see the real benefit with the aspect of the
> SWIG interface.
>

All mapscripts had mapObj.setWeb until last week, I am only trying to
make it work. Programs that did not work now will do. I don't think it
breaks any backward compatibility, but please correct me if I'm wrong.

Regards,
Umberto

> Tamas
>
>
>
> 2006/8/23, Umberto Nicoletti <umberto.nicoletti at gmail.com>:
> > Adding this line of code to map.i (and exposing msCopyWeb in map.h):
> >
> >     void setWeb(webObj* web) {
> >         if (web) {
> >                 msCopyWeb(&(self->web), web, self);
> >         }
> >     }
> >
> > is an alternative solution to this problem which fixes the double free
> > problem and lets the user create an empty web object and later add it
> > to a map.
> > msCopyWeb is already present in mapcopy.c because it is used in msCopyMap.
> >
> > This copy strategy is already widely used in all 'safe' mapscript
> > functions and in my opinion we should keep walking along this path.
> >
> > If you agree I will commit this changes to cvs as a fix for 1869 and
> > after 4.10 is out we can think of standardizing this strategy for
> > other cases too.
> >
> > Best regards,
> > Umberto
> >
> > On 8/21/06, Steve Lime <Steve.Lime at dnr.state.mn.us> wrote:
> > > Let's wait to hear from the users to see if this becomes an issue before
> > > condemning
> > > the decision. Based on the information at hand it seems the correct
> > > move to me. I also
> > > think it important to keep the languages in sync if at all possible. If
> > > something simply
> > > won't work in one binding when there is a reasonable, even preferable,
> > > alternative
> > > that will work with all bindings then we should opt for consistency.
> > >
> > > Again, don't let this one issue get in the way of moving forward.
> > >
> > > Steve
> > >
> > > >>> Umberto Nicoletti <umberto.nicoletti at GMAIL.COM> 8/21/2006 3:39:03
> > > AM >>>
> > > Hi all,
> > > got back today and I have already a bug report because of the changes
> > > approved in this thread: an API change in 4.10 and probably another in
> > > 5 is not too respectful of our users I guess.
> > >
> > > Now since the majority of the devs has voted in favour of the proposal
> > > and I do not have veto rights I'll go on and downgrade the Java
> > > examples to the newer functionality, but needless to say I'm *very
> > > disappointed* and I'll consider proposing to make some of these
> > > changes c-sharp specific since the fatal errors that Tamas seems to
> > > experience in c-sharp I cannot reproduce them in Java.
> > >
> > > Umberto
> > >
> > > On 8/18/06, Tamas Szekeres <szekerest at gmail.com> wrote:
> > > > Along with Frank's +1 on the IRC we have at least +3 so i have
> > > > committed the changes.
> > > > HISTORY.TXT contains the interface elements affected.
> > > >
> > > > /me thinks it was a bit sweaty but was worth the trouble
> > > >
> > > > Tamas
> > > >
> > > >
> > > > 2006/8/17, Steve Lime <Steve.Lime at dnr.state.mn.us>:
> > > > > +1 from me too. The nice thing it's a beta release so there's still
> > > time
> > > > > if we
> > > > > find we've overstepped a boundary. This is good discussion and I
> > > want
> > > > > to
> > > > > thank Tamas for the investigative work and solution (and Umberto
> > > for
> > > > > taking
> > > > > time from vacation - you're more generous than I!)...
> > > > >
> > > > > Steve
> > > > >
> > > > > >>> Daniel Morissette <dmorissette at MAPGEARS.COM> 8/17/2006 4:03:53
> > > PM
> > > > > >>>
> > > > > After a bit more thinking and discussion on IRC, I am with Steve
> > > and
> > > > > also think that all the members that are listed in Steve's email
> > > can
> > > > > safely be made immutable since it would not make sense to attempt
> > > to
> > > > > overwrite them anyway. (That's also the way things are in PHP
> > > MapScript
> > > > >
> > > > > and that never stopped us from doing anything).
> > > > >
> > > > > With respect to imageObj.format, I think it should be made
> > > read-only or
> > > > >
> > > > > hidden since we get an imageObj from methods that create the
> > > actual
> > > > > image, and changing the outputFormatObj on the image after it's
> > > been
> > > > > created is just asking for trouble (not to mention the possible
> > > object
> > > > >
> > > > > referencing issues that Tamas is trying to address).
> > > > >
> > > > > I give my +1 to go ahead with this.
> > > > >
> > > > > Also please make sure you include a note listing the members that
> > > were
> > > > >
> > > > > made immutable in HISTORY.TXT.
> > > > >
> > > > > Daniel
> > > > >
> > > > > Steve Lime wrote:
> > > > > > Umberto: How do you add classes and layers at runtime? In the
> > > context
> > > > > of
> > > > > > a layer or a map
> > > > > > correct? I don't believe that what Tamas is proposing would
> > > change
> > > > > that
> > > > > > capability at all.
> > > > > >
> > > > > > If you look at the list:
> > > > > >
> > > > > > classObj.layer
> > > > > > webObj.map
> > > > > > legendObj.map
> > > > > > layerObj.map
> > > > > >
> > > > > > are back references and you don't want folks monkeying around
> > > with
> > > > > > them.
> > > > > >
> > > > > > classObj.label
> > > > > > legendObj.label
> > > > > > mapObj.scalebar
> > > > > > mapObj.legend
> > > > > > mapObj.querymap
> > > > > > mapObj.web
> > > > > > mapObj.symbolset
> > > > > > mapObj.labelcache
> > > > > > mapObj.reference
> > > > > >
> > > > > > Already exist in their parent objects so there is no need to
> > > create
> > > > > new
> > > > > > ones in place of the
> > > > > > existing ones. Users should get the reference to the existing
> > > > > structure
> > > > > > and modify it in place.
> > > > > > I'm not sure that given the potential problems that it even
> > > makes
> > > > > sense
> > > > > > to have constructors
> > > > > > for these. I assume this is the contentious spot...
> > > > > >
> > > > > > (If there is a glaring need to have, for example, an unattached
> > > > > > labelObj laying around then
> > > > > > couldn't a populateFrom capability be developed for a labelObj,
> > > or
> > > > > > perhaps setLabel() for a
> > > > > > classObj?)
> > > > > >
> > > > > > classObj.metadata
> > > > > > fontSetObj.fonts
> > > > > > mapObj.configoptions
> > > > > > webObj.metadata
> > > > > >
> > > > > > Are hash tables and Sean created a nice interface for them to
> > > use.
> > > > > > Modifying directly shouldn't
> > > > > > be allowed. I think this is what was ok'd yesterday.
> > > > > >
> > > > > > Note sure about  imageObj.format...
> > > > > >
> > > > > > I guess I don't see the big deal with this change. My 2 cents.
> > > > > >
> > > > > > Steve
> > > > > >
> > > > > >
> > > > > >>>>Umberto Nicoletti <umberto.nicoletti at GMAIL.COM> 8/17/2006 8:25
> > > AM
> > > > > >>>>
> > > > > >
> > > > > > On 8/17/06, Daniel Morissette <dmorissette at mapgears.com> wrote:
> > > > > >
> > > > > >>Tamas Szekeres wrote:
> > > > > >>
> > > > > >>>>I think you can safely hide the labelPathObj, that should not
> > > be
> > > > > >>>>exposed. I think the others
> > > > > >>>>should be immutable too (assuming I understand the problem
> > > > > >
> > > > > > correctly),
> > > > > >
> > > > > >>>>the question is how
> > > > > >>>>many scripts will the change break and should backward
> > > > > >
> > > > > > compatability
> > > > > >
> > > > > >>>>breakage be limited
> > > > > >>>>to major releases (e.g. 5.0).
> > > > > >>>>
> > > > > >>>
> > > > > >>>Those scripts are *erroneous* !  It would be desirable to force
> > > > > >
> > > > > > the
> > > > > >
> > > > > >>>users to put them into a good shape as soon as possible.
> > > > > >>>
> > > > > >>>
> > > > > >>
> > > > > >>It seems that I should take position as release manager but
> > > > > >>unfortunately I do not know or use SWIG MapScript much so I have
> > > to
> > > > > >
> > > > > > rely
> > > > > >
> > > > > >>on those who know to make an opinion on whether the current stuff
> > > is
> > > > > >>dangerous enough to warrant breaking a few scripts with 4.10.
> > > > > >>
> > > > > >>Based on the understanding I have of the problem from reading
> > > this
> > > > > >>thread and previous discussions, it seems to me that if those
> > > > > >
> > > > > > scripts
> > > > > >
> > > > > >>are doing something that is just waiting to bomb then changing
> > > SWIG
> > > > > >>MapScript in v4.10 to make those object references immutable is
> > > not
> > > > > >
> > > > > > a
> > > > > >
> > > > > >>backwards compatibility issue, it is a bugfix and a service we
> > > are
> > > > > >>making to those users by forcing them to fix their scripts... and
> > > in
> > > > > >
> > > > > > the
> > > > > >
> > > > > >>end their apps will just be more robust.
> > > > > >>
> > > > > >
> > > > > >
> > > > > > Daniel,
> > > > > > IMO  we are talking about fixing pieces of code we are not even
> > > sure
> > > > > > they are broken. For instance I have some Java mapscript tests
> > > (and
> > > > > an
> > > > > > application) running just fine that add layers and classes at
> > > run
> > > > > time
> > > > > > and that's why I am so reluctant in approving this proposal.
> > > Only
> > > > > once
> > > > > > we get a test case that reproduces this errors reliably then we
> > > can
> > > > > go
> > > > > > and change the code.
> > > > > >
> > > > > > Unfortunately I couldn't join the IRC meeting because I am on
> > > > > vacation
> > > > > > otherwise I'd have spoken  earlier (I am writing this on the
> > > road).
> > > > > >
> > > > > > Regards,
> > > > > > Umberto
> > > > > >
> > > > > >
> > > > > >>The real question that I would throw at the SWIG MapScript
> > > experts
> > > > > >
> > > > > > is:
> > > > > >
> > > > > >>"Is the practice of overwriting those object references really
> > > > > >
> > > > > > dangerous
> > > > > >
> > > > > >>or not?"
> > > > > >>
> > > > > >>If the answer is yes then I think that solves the question and
> > > we
> > > > > >
> > > > > > need
> > > > > >
> > > > > >>to apply the fix and force users to fix their scripts.
> > > > > >>
> > > > > >>If there are object references that can safely be overwritten
> > > (such
> > > > > >
> > > > > > as
> > > > > >
> > > > > >>colorObj) then my opinion is that we should not touch them in
> > > 4.10.
> > > > > >>
> > > > > >>Daniel
> > > > > >>--
> > > > > >>Daniel Morissette
> > > > > >>http://www.mapgears.com/
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Morissette
> > > > > http://www.mapgears.com/
> > > > >
> > > >
> > >
> >
>

More information about the mapserver-dev mailing list