Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009

Subject: Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009
From: Thomas Klimpel (Thomas.Klimpel_at_[hidden])
Date: 2009-08-26 18:29:15

Simonson, Lucanus J wrote:
> Thank you for your careful reading of the examples and some of their associated documentation.
>  Are you suggesting that I should add more explanation
> (in the form of comments) to the example code?

No, it's more something along the line: "I want to read an introduction/tutorial for the library, so what am I supposed to do?" I figured out that the examples are meant to be some sort of tutorial. I worked quite well for me, only the last two examples were a bit too difficult, so I tried to read the associated documentation for them.

Apart from that, I was just looking for an excuse to write my observations without further clarifications.

> > - point_usage.cpp:
> > assert(gtl::euclidean_distance(pt, pt2) == 10.0f);
> > is not a good idea, because floating point comparison will not work
> > out
> Floating point comparision does work and the assert is not triggered when the code is run.

I can't believe that there is no C++ compiler out there that will trigger "assert(sqrt(100.0f) == 10.0f);" for suitably chosen compiler flags.

> > - gtl_polygon_usage.cpp:
> > assert(gtl::extents(rect, poly)); //get bounding box of poly
>
> I see nothing wrong with this line.

I guess "gtl::extents(rect, poly)" is meant to set rect to the bounding box of poly. So it's probably not a good idea to wrap the call inside an assert.

> > - custom_polygon.cpp:
> > static inline unsigned int size(const CPolygon& t) {
>
> Technically this line should return std::size_t, is that what you are pointing out?

Yes, because "unsigned int" is quite unexpected as return type here. Just returning std::size_t will be more smooth to read for the newcomer, even if the library can also accept unsigned int as return type.

> > - connectivity_extraction_usage.cpp:
> > assert(ce.insert(test_data[i]) == i);
>
> Not sure what you are calling out.  The id assigned by insert starts at zero and increments, meaning it happens to be equal to i in my example code.

I don't think that the call should be wrapped inside an assert.

> > //Now you know how to use the connectivity extraction algorithm
> > //to extract the connectivity graph for overlapping geometry
>
> Not sure what you are calling out.
>
> > - property_merge_usage.cpp:
> > //Now you know how to use the manhattan and arbitrary angle property
> > //merge algorithms to perform map overlay on n layers of input
> > geometry
>
> Not sure what you are calling out.

The statement "Now you know how to use the ..." wasn't true for me..

> > coordiante_type should be coordinate_type, but what's the meaning of
> > "polygon_set_data<coordinate_type>"?
>
> polygon_set_data<coordinate_type> is a class defined in the library documented in the polygon set concept page.

I found it. Thanks.

- gtl_property_merge_45.htm still has some typos in it:
property_merge_45(const property_merge_90& that)
should probably be
property_merge_45(const property_merge_45& that)

The context menu for "Property Merge 45" is still inconsistent, because of "<p>&nbsp;</p>" in line 42, and because of "<li>Property Merge_90</li>" instead of "<li>Property Merge 90</li>".

All other *.htm files have "<li>Property Merge_45</li>" instead of "<li>Property Merge 45</li>", which should probably also be fixed.

Regards,
Thomas