[mapguide-internals] Some code fixes for RFC 60

[Traian Stanev]

I am looking at the RSCOLORLIST structure in the RenderingService part of the RFC (v8 of the patch file) and I have some comments.

Using pointers allocated with new to point to something as small as a color is not "more flexible". It is:

* A lot slower. Probably at least 10x slower, due to all the calls to new().
* Uses a lot more memory. A color can be represented in 4 bytes, using RS_Color::argb(). You are using 4 bytes for the pointer, 16 bytes for the color, plus the malloc() overhead per color, which is at least 8 more bytes. 
* A lot more complicated and leak-prone, in that it requires you to write more code to free the colors when you are done with them.

Here is what you could have done:

std::vector<unsigned> colorList;

This way:

* You don't need to allocate the list using new(). 
* You don't need to allocate list members using new(). 
* You don't have to worry about catching exceptions in order to delete the list pointer you allocated using new(), since the destructor of the vector will automatically get called when it goes out of scope.
* You use far less memory and your code is a lot simpler and a lot faster.
* Since the type is not typedef-ed by an obscure name, another programmer would know immediately what data structure he is dealing with (a vector of integers).

Also, you did not follow the style of the rest of the code by declaring all-caps data types, in defiance of pretty much every single type in the rest of the project...

Thanks,
Traian


-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
Sent: Friday, May 08, 2009 8:29 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Some code fixes for RFC 60

Thanks for the input. Much appreciated.

Added the delete.

Why did I use pointers?
I did use pointers as this leaves more flexibility for later improvements.
Also In the beginning I was not sure if the map object might be shared 
between threads or sessions.
I can imagine some use cases where this might be an issue.... at that 
stage different colorpalettes need to be handled and pointers would be 
more flexible.

To notice an eventual leak in the assignment I added the assert 
statement....
so its an odd coding but a destructive assignment does not leak it 
breaks ;) 
This is like a runtime check to understand whats happening in a 
multithreaded server.
So its the same  for now.

I added those things now.... the Coordsys error message (#980) is also 
included.
I  commented out the experimental exception throw in StylizeLayers...

Cheers,
UV


Walt Welton-Lair wrote:
> UV,
>
> Two fixes:
>
> * Change your GetResourceServiceForMapDef method in ServerTileService to simply return an MgResourceService* (properly addref'ed) and not a smart-pointer.  We don't have any methods which return smart-pointers.  It's up to the calling code to decide if it wants to wrap the method's return value in a smart-pointer.
>
> * In MapLayer/Map.cpp you have numerous memory leaks.  You allocate a new STRCOLORLIST (assigning it to the m_colorPalette member variable), but don't delete it anywhere.  Instead you only call clear on the list, which isn't the same as deleting the object.  SetColorPalette also leaks any previously allocated m_colorPalette.  I would simply change m_colorPalette to be a STRCOLORLIST instead of a PSTRCOLORLIST.  GetColorPalette then returns the address of m_colorPalette, and SetColorPalette replaces the contents of m_colorPalette with the supplied palette.
>
> Thanks,
> Walt
>
>  W       E         @__   __o
>    A   T   R   @___    _ \<,_
>      L           @_   (*)/ (*)
>
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>   

_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals