[mapguide-internals] Additional RFC60 review

[Walt Welton-Lair]

Quick update...  I think UV sufficiently addressed Trevor and my review comments, and I plan to merge the RFC60 changes into trunk sometime this week.

Walt

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
Sent: Tuesday, December 22, 2009 7:38 PM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Additional RFC60 review

Thanks for the input.

In Revision 4462 I fixed the addressed issues mostly.
I left a few bits which I would rather call documentation.

This is the last contribution I can make for a while as I will be moving
on to some other (paid) things now.

Keep up the good work!  .... and embrace change!


Walt Welton-Lair wrote:
> Hi UV,
>
> I've spent most of today reviewing your RFC60 changes.  I agree with Trevor's
> review comments, and I saw that you have already addressed most of them.
>
> A note regarding tab characters, I plan to eventually rerun my utility
> which cleans these up.  I could wait to run it until after you have
> merged and then it will fix your changes as well.  That way you don't
> have to do it this time.  For future submissions though, please make
> sure your code editor is configured to insert spaces and not tabs.
>
>
> Here are my comments.  There are a lot of them, but they're all fairly
> straightforward to address.  These assume your latest RFC 60 code.
>
> Oem
> - I assume nothing in this folder will be merged.
>
> BuildTools\cc.net\ccnet.config
> - Should not be merged.
>
> Common\MapGuideCommon\MapLayer\Map.h
> - You mentioned m_colorPalette is a lazy instantiated container.  But both
>   MgMap constructors currently allocate a container rather than initialize
>   the pointer to NULL, so it's not lazy at all.
>
good point.... now it is.
> Common\MapGuideCommon\MapLayer\Map.cpp
> - There are some memory leaks:
>   * MgMap::Create(MgResourceService* ...) has a leak.  If the color
>     palette exists but is empty, a new one is allocated and the pointer is
>     overwritten without deleting the old object (if it exists).
>   * MgMap::Create(CREFSTRING ...) creates a memory leak.  The constructor
>     allocates m_colorPalette, and this method clears the pointer without
>     first deleting the existing object.
>   * Inadvertent errors like this is a good reason to make m_colorPalette
>     be a ColorStringList, like Trevor originally suggested.  I recommend
>     changing it as well.  It simplifies the code and removes any chance of
>     a leak.  The lazy instantiation performance improvement is not worth it
>     IMO.
> - Me just nitpicking: don't add extra unnecessary newlines (line 411, 451)
> - No need to clear the palette in MgMap::Dispose, since destructor will do it.
> - In AddColorsToPalette you filter empty strings before calling std::transform,
>   but still add the empty strings to the color palette.  Should they not be
>   added?
> - You have calls to std::sort and std::unique in GetColorPalette.  You
>   should therefore remove the sort/unique calls in ParseColorStrings in the
>   rendering service, since it calls GetColorPalette.
> - Me just nitpicking: line 1144 - put the comment on a separate line.
>
>
done
> Common\MapGuideCommon\Services\RenderingOptions.cpp
> - Looks good.
>
> Common\Renderers\AGGImageIO.cpp
> - Includes should never be added before the include of "stdafx.h".  When
>   precompiled header usage is enabled, any includes before stdafx.h are
>   ignored.
> - Is the include of ace/OS.h needed when _DEBUG_PNG8 is turned on?  If so
>   then wrap the include in an ifdef.
> - Add back include of AGGRenderer.h (see notes for AGGImageIO.h).
> - Remove commented include of smartalloc.h.
> - Add a brief comment explaining the purpose of the UseColorMap static.
> - In CreateGdImageWithPalette, declare gdPPaletteFromImg24 in the block
>   where it's actually used.
> - In CreateGdImageWithPalette, get rid of commented calls to delete.  They
>   distracting, as well as incorrect.
> - The use of your myassert macro can lead to memory leaks.
>   * In CreateGdImageWithPalette, the third call to myassert can cause a leak.
>     Since the myassert macro throws an exception, the previously allocated
>     gdPPalette pointer ends up getting leaked.  Same problem with the fourth
>     call to myassert in this method.
>   * There are two calls to myassert in one of the AGGImageIO::Save overloads.
>     Both result in leaks if the assert goes off.  For the first, the gdImage24
>     pointer gets leaked, while for the second, the gdImage24 and gdImgPalette
>     pointers get leaked.
>   * Do you really want myassert to raise an exception?
> - Lines 1011-1014: get rid of commented "former GIF code"
> - Me just nitpicking: lines 962, 1015 - put comments on separate lines.
> - Always keep at least one blank line between methods (preferably two).
>
>
done
(exception only thrown for debug build --> never mind the leak)
> Common\Renderers\AGGImageIO.h
> - Remove the include of AGGRenderer.h (it's no longer needed now that you've
>   moved the RS_ColorVector definition to RenderStyles.h).
> - We also want to avoid including gd.h in AGG class headers.  The use of GD
>   within AGG is an implementation detail which shouldn't be exposed through
>   the headers.  To make your code work without gd.h in the header, you can
>   either change the gdImagePtr parameters to void* (see for example GDRenderer's
>   m_imgout and m_imsym which also do this), or just change the new static
>   methods to be helper methods declared directly in AGGImageIO.cpp (works
>   since they're not referenced outside of AGGImageIO.cpp).  I prefer the
>   latter.
>
>
done
> Common\Renderers\AGGRenderer.cpp
> - Separators missing before SupportsTooltips, ApplyLineStyles, ProcessArea,
>   DrawScreenPolyline, AddExclusionRegion
> - Always keep at least one blank line between methods (preferably two).
> - Use slashes for method separators.
>
> Common\Renderers\AGGRenderer.h
> - Looks good.
>
> Common\Stylization\DefaultStylizer.cpp
> - Me just nitpicking: always keep at least one blank line between methods
>   (preferably two).  E.g. StylizeGridLayer doesn't have any in your version
>   of the code.
>
>
this project should consider getting a formatting tool !!
such a thing could sit in the CI build system and make this whole issue
obsolete
> Common\Stylization\DefaultStylizer.h
> - For GetSymbolManager move the implementation to the cpp file.  I realize
>   it's just a one-line implementation, but for consistency I always put
>   implementations in cpp files so people always know where to look.  And use
>   newlines in the implementation.  I remember some older versions of Visual
>   Studio would not let you hit breakpoints set on methods that were declared /
>   implemented on a single line.
>
VS2008 is the required minimum. I navigate with definition of context
menu?!? so does it matter?
> Common\Stylization\RendererStyles.h
> - Get rid of the gdImage and gdImagePtr typedefs.  These are only used within
>   the Renderers project, and in there the typedefs are already available via
>   GD.
>
>
done
> Server\Src\Common\Manager\Makefile.am
> - See comments ServerRenderingService.h.  If you remove the AGGRenderer.h
>   include from ServerRenderingService.h then no changes are needed to this
>   file.
>
>
done
> Server\Src\Common\Manager\ServerManager.vcproj
> - If you remove the AGGRenderer.h include from ServerRenderingService.h
>   then no changes are needed to this file.
> - General note: when making changes to project file configuration settings
>   don't forget to update ALL configurations.  You only made changes to the
>   Win32 configurations and didn't update the x64 configs.
>
>
changes removed.... good point ....
> Server\Src\PostBuild\PostBuild.mak
> - This won't be merged, as mentioned in your response to Trevor's review
>   comments.
>
ditto
> Server\Src\Services\Mapping\MappingUtil.cpp
> - Was there a good reason to reorder all the includes?  Linux builds are
>   sensitive to the include order (much more than Windows).
> - The myassert define is not used in this file, so remove it.
> - Comment on line 559: an FdoPtr isn't an autoPtr
> - Lines 572-580: move the printf calls inside the if (extractColors) block.  GOOD POINT
> - Lines 843-844: remove commented code
> - Line 1335: remove commented code "bool isRef = false;"
> - Comment line 1369: "the property setter canonicalizes the palette".  Not
>   anymore with your latest changes.
> - In ExtractColors, no need to check for an empty ColorStringList since
>   AddColorsToPalette already does.
> - Calls to Rule::GetLabel - the returned label can be NULL and the code needs
>   to check for this.
> - Calls to xxRule::GetSymbolization 0 the returned object can be NULL and the
>   code should check for this.
> - When adding colors to the ColorStringList you're calling substr(), I think
>   to create a copy of the string since the MdfStrings returned from the API
>   are references.  But isn't a copy automatically made when the string is added
>   to the list?  Have you stepped into the std::list::push_back call to verify
>   that substr is actually needed?
> - FindColorInSymDefHelper methods: is there honestly any benefit in declaring
>   these as inline?
> - In GraphicElementVisitorImpl, add TODO comments for VisitImage and VisitText.
>   These can be implemented by someone at a later time.
> - In the CompoundSymbol version of FindColorInSymDefHelper add a TODO comment
>   for the case where the symbol is defined via a resourceID reference.  This can be
>   implemented at a later time.
> - Curly braces go on new lines, per our coding style.
>
>
done
> Server\Src\Services\Mapping\MappingUtil.h
> - Looks good.
>
> Server\Src\Services\Mapping\ServerMappingService.vcproj
> - Does this file actually need to be changed?
> - If so, only the Debug|Win32 configuration is currently updated, and all four
>   configurations need to be updated.  Also, the Makefile.am file would need
>   to be updated.
>
changes removed
> Server\Src\Services\Rendering\ServerRenderingService.cpp
> - Use newlines for implementation of hasColorMap().
> - In your latest change you uncommented the new calls to MG_TRY / MG_CATCH
>   (I don't think Trevor realize you had originally added them as commented
>   code).  The only renderer which actually does cleanup in EndMap is
>   DWFRenderer, so it could benefit from this change.  The calls are not needed
>   though to skip faulty tiles (as mentioned in your code comment).  If an
>   exception is raised in RenderMapInternal while generating a tile it will be
>   caught in RenderTile, and your RFC60 code will not be executed.  Two
>   additional changes you need to make:
>   * You should use the MG_THROW macro and not re-throw the exception yourself.
>     Your code also has a reference count bug (look at MG_THROW).
>   * In the current code the exception is re-thrown only for debug.  It must
>     always be re-thrown, and a server error will be logged further up in
>     the call stack.
> - Issues with the RS_ColorVector:
>   * The tileColorPalette pointer is not initialized to NULL.  In the case
>     of an exception occurring during the Save call while using GD, the code
>     (as it's currently written) will try to delete an uninitialized
>     pointer.  While this code path should not currently happen, it may in
>     the future.
>   * There's a big memory leak: in the case of AGG the RS_ColorVector object
>     is allocated, but it's only getting deleted in the exception handler.
>   * Please clean all of this up by allocating the RS_ColorVector object
>     on the stack in the case of AGG.  This is faster and it simplifies the
>     code.
> - Line 955 which checks for the AGG renderer also now includes a check for
>   hasColorMap.  This logic is wrong.  If the renderer is AGG but hasColorMap
>   returns false then your code will jump to the else block and use GD
>   renderer.
> - It looks like the RFC60 code in AGGImageIO raises std::exceptions, and
>   these are rethrown here.  While the MG_CATCH macro will handle these,
>   does this provide sufficient information in the error log?  Have you
>   checked this?
> - The coding style of the "rfc60 code to correct colormaps" section needs
>   work:
>   * curly braces go on new lines
>   * code alignment is messed up (some lines aren't indented, others are
>     indented 3 characters)
>   * tab characters need to be replaced with spaces
> - You added code to throw a MgNullArgumentException in the case where the
>   renderer did not return data.  This type of exception is used when
>   "a null argument is passed to a method that expects a non-null value".
>   A better one might be MgStylizeLayerFailedException.  In any case, why
>   do we need to throw the exception?  Is it to log an error?  Is this a
>   requirement of RFC60, or is it to address some other issue?  Also,
>   put the throw call on a new line so it's easy to set a breakpoint on
>   that specific line.
> - Always keep at least one blank line between methods (preferably two).
>
done
> Server\Src\Services\Rendering\ServerRenderingService.h
> - Now that you've moved the RS_ColorVector definition to RenderStyles.h,
>   you can get rid of the include of AGGRenderer.h.  This lets you avoid having
>   to update the ServerManager project include paths.
> - If you also move ParseColorStrings to StylizationUtil as Trevor suggested,
>   then no changes are needed to this file.
>
> Server\Src\Services\Tile\ServerTileService.cpp
> - You changed a reference to MgUnableToOpenTileFile to MgUnableToOpenLockFile.
>   Looking at where MgUnableToOpenTileFile is used, it does seem to always
>   refer to an error opening the lock file.  So this change seems ok, but:
>   * also update the other two places where it's used
>   * you must also update the resource file
>     - file is at Common\MapGuideCommon\Resources\mapguide_en.res
>     - update the name of the resource
>     - update content/value: "Unable to open the tile lock file." sounds
>       reasonable
>   * get rid of the "MgUnableToOpenTileFile???? sounds wrong" comment
> - Get rid of "refactored from below" comment and remove the commented code.
>   Your code improvement is good.
> - Always keep at least one blank line between methods (preferably two).
> - Use slashes for method separators.
>
>
done
> Server\Src\Services\Tile\ServerTileService.h
> - I agree with Trevor's comments for this file.  The GetResourceServiceForMapDef
>   method would definitely be worthwhile if it was called from at least two
>   places.  But it's only used in one place and it's relatively small method,
>   so IMO there's no tangible benefit.
>
> Server\ServerStructured.sln
> - Should not be merged.
>
We always had a number of different solutions in our VS repositories.
Targeted at different uses or users.
This one uses folder structures which are clean and neat.
nobody needs to use it..... why prohibit anyone else from using it and
force to use the existing one with a way too long project list???
maintaining additional solution files is a minor effort - the projects
stay the same....
>  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