[boost] GGL Review

Subject: [boost] GGL Review
From: Michael Caisse (boost_at_[hidden])
Date: 2009-11-21 23:10:33

Let me start by apologizing for the short amount of time I was able to put
into this review. I have spent about 5 hours reviewing the documentation,
looking at some of the source and trying a few examples of mine own. Clearly
an enormous amount of work has gone into this library and I feel like it
deserves more of my time than I am able to give right now. Nevertheless, I
thought it important to review the most critical portions of the library.

Based on the library review announcement, GGL is promoting key concepts
for describing geometries. Some algorithms are provided. As such, it seems
to me that the interfaces and concepts are key and is what I have devoted my
time to looking at.

And now for the executive summary spoiler: I believe that this library offers
well defined concepts for geometry. The design is sound and the extension
capabilities appear to be reasonable based on my short trial. I found the
interface and concepts to the library usable and therefore recommend
acceptance of this library within Boost. That was a wordy way of saying it has
a Yes vote from me.

===== What is your evaluation of the design? =====

I am quite pleased with the use of tag-dispatching and strategies. I was
happy to see this approach taken and by looking at the documentation and
source code it seems sound. I believe this approach offers the greatest
latitude for a generic library.

===== What is your evaluation of the implementation? =====

Very clean. Every file I opened and took a look at was readable and easy to
follow. The directory structure is obvious and made exploring the library
an easy feat.

I would like to see ggl::exception inherit from boost::exception
instead of std::exception.

I was unable to spend time evaluating the implementations of
algorithms in any detail.

===== What is your evaluation of the documentation? =====

I found the "Design rationale" section well written and quite applicable to the
problem at hand. According to the review announcement:
     "GGL defines concepts for geometries and implements some algorithms on such
      geometries."

As such, the concepts are paramount to the review and the design rationale provides
more than tidbits about the underlying structure of the library. It provides the
requirements for compliance to the concepts promoted by the library.

Adding generality step-by-step while using a basic concept such as distance
allows the reader to follow the logic behind the tag-dispatching and quickly
see how extensions and modifications could be incorporated.

I am not typically a fan of Doxygen generated docs for generic libraries. Much
has already been said about this by other reviewers; however, I did not find
the documentation as lacking as some have described. I think the current
state is adequate for using the library and did not encounter any problems
using the docs while writing my own small tests. I can see that some
of the descriptions are a little terse and if I was trying to implement
something more complex I might have questions.

A few specific comments on the docs.........................

design.html section "Return type"

typename Strategy::return_type distance <http://geometrylibrary.geodan.nl/formal%5Freview/group__distance.html#g22a618786d2601e9201896a8346c161b>( G1 const& G1 , G2 const& G2 , S const& strategy)

  -- should be --

typename Strategy::return_type distance <http://geometrylibrary.geodan.nl/formal%5Freview/group__distance.html#g22a618786d2601e9201896a8346c161b>( G1 const& g1 , G2 const& g2 , S const& strategy)

and

inline typename distance_result<G1, G2>::type distance <http://geometrylibrary.geodan.nl/formal%5Freview/group__distance.html#g22a618786d2601e9201896a8346c161b>(G1 const& G1, G2 const& G2)

  -- should be --

inline typename distance_result<G1, G2>::type distance <http://geometrylibrary.geodan.nl/formal%5Freview/group__distance.html#g22a618786d2601e9201896a8346c161b>(G1 const& g1, G2 const& g2)

design.html section "Multi"

The comments at first confused me. They are correct but by the end of reading a whole page
on type traits my eyes were getting tired. Perhaps something like this for clarification:

struct distance <http://geometrylibrary.geodan.nl/formal%5Freview/group__distance.html#g22a618786d2601e9201896a8346c161b>
    <
        GeometryTag1, GeometryTag2, G1, G2, Strategy,
        false, // G1 is_multi result
        true   // G2 is_multi result
    >

===== What is your evaluation of the potential usefulness of the library? =====

I think a library implementing generic geometry concepts is extremely
useful in many domains. I think this library provides usable concepts
for a geometry library with a design and implementation that allows extension
to user types and algorithms.

===== Did you try to use the library?  With what compiler?  Did you have any
      problems?                                                               =====

I found that the examples did not compile using the normal method. I was using the
boost_1_41_0 release.

        cd libs/ggl/example
        bjam release

Changing the Jamfile include requirement to <include>../../.. makes it work fine.

I compiled with VC8, gcc-4.3.3, and gcc-4.3.2 m68k-elf cross-compile. The cross-compiler
is for a device without FPU and I will continue looking at this library for integer
only calculations next week.

gcc-4.3.3 spat a bunch of warnings about "use of C99 long long integer constant" from
boost/integer_traits.hpp. Not a GGL issue.

The 03_polygon_example.cpp tickles some uninitialized warnings with gcc-4.3.3 that originate
from intersection.hpp.

===== How much effort did you put into your evaluation? A glance? A quick
      reading? In-depth study?                                             =====

I spent about 5 hours reviewing the library with a careful read of the
Main Page, Design rationale, Spatial set-theoretic operations, and Relation
to Boost and std libraries.

I also looked over the source code, examples and implemented my own small test cases.

===== Are you knowledgeable about the problem domain? =====

Somewhat. I spent almost 10-years working with GDS-II and other semiconductor
mask formats. I worked on transforming data at the "server" level and writing microcode
for very accurate and high speed rendering on custom hardware. I have about
a year experience working with GIS data sets. I can appreciate the integer versus
floating point discussion that has been going on. Different domains have
very different requirements. Those who promote the idea that integer only
is pointless or floating point is THE solution haven't worked in the other
domains. One is not more important than the other; however, concepts described 
by a library should be able to adapt to either. This library obviously
performs in the FP realms and I have no problem envisioning how to adapt
types and algorithms to work with it for integer only problems. I hope
to explore this domain further in the coming weeks with GGL.

Thank you GGL team for your hard work. Thank you Hartmut for your
willingness to manage this review.

Best regards -
michael

-- 
----------------------------------
Michael Caisse
Object Modeling Designs
www.objectmodelingdesigns.com