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

Subject: Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009
From: Simonson, Lucanus J (lucanus.j.simonson_at_[hidden])
Date: 2009-08-26 20:24:14

Thomas Klimpel wrote:
> 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>
<li>Property Merge 90</li>
" instead
> of "".    
> 
> All other *.htm files have "<li><a
> href="gtl_property_merge_45.htm">Property Merge_45</a></li>" instead
> of "<li><a href="gtl_property_merge_45.htm">Property Merge
> 45</a></li>", which should probably also be fixed.   
> 
> 
> Regards,
> Thomas
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost