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-29 15:56:06

Mathias Gaunard wrote:
> I wonder how difficult it would be to extend the system so that it
> could 
> work with statically-sized polygons. (the library only provides
> rectangles, basically polygons 90 which a static size of 4)
> I'm thinking of triangles in particular.
> Also, could algorithms like triangulation eventually become part of
> the library?

You could make a polygon_traits specialization for your own triangle class, but not a polygon_mutable_traits.  This would allow the library to view your triangle type as a read-only polygon type.  Triangle concept and related algorithms such as triangulation could eventually become part of the library, I would welcome such as community contributions.
 
> Still in the static vs dynamic argument, it appears points implement
> by default the static coordinate lookup (the x() and y() functions) in
> terms of the dynamic one (the get(orientation_2d) function).
> Shouldn't it be the other way around?

The assumption here is that when the dynamic one is instantiated and inlined by the compiler constant propigation will allow it to optimize it away producing an interface without runtime overhead.  

> The documentation says euclidean_distance was named so instead of
> distance to avoid ambiguity with std::distance.
> Isn't that problem simply solved by qualifying the call? Or maybe I'm
> missing something?

No, the thing you are missing is ADL.  The existence of distance() in my own namespace causes ADL lookup to do the wrong thing when instantiating stl templates that use std::distance() on iterators in older version of the standard library.

> Personally, I tend to prefer functions that return results rather than
> those that take some input and modify it, since the latter require the
> objects to model the mutable variant of the concept, and mutation
> makes 
> code more complex to reason about by undermining referential
> transparency and equational reasoning. (part of that sentence was
> copy/pasted ;)).
> Couldn't a function like "center", for example, instead of taking a
> point to modify, directly return some kind of simplistic expression
> template, then perform the evaluation when a conversion to T is
> requested, using T as an output type?

I agree with you about equational reasoning.  Center used to return point_data.  I changed it to be consistent with other interfaces and allow user defined point types.

As to your suggestion, I can't implement a generic assignment operator or copy constructor for T, so what you are suggesting would not eliminate the temp copy I'm trying to avoid by passing many outputs by reference as the first argument.  In the case of a point, returning an object that casts to arbitrary point type is reasonable because the temp copys are cheap and easy for the compiler to optimize away.  In the case of polygons or collections of polygons this is not the case.

> Finally, about the operator overloading for set operations, I would
> like 
> to be able to avoid pushing the namespace in the global one if
> possible. 
> I think it would be nice to have a "ref" function or something that
> would make objects just behave like a reference to the original object
> but within the right namespace so that the operators get picked up by
> ADL. Functions that would return expression templates like discussed
> above 
> could also make their return type in the right namespace so that it
> gets picked up by ADL too.

I can't say I prefer

ref(A) | ref(B)

to 

A | B

Usually I pull the operators namespace into the local scope of a function where they are used and not the global scope.  Pulling them into a scope that defines other operators can actually cause ADL problems that prevent them from being found since ADL ignores using directives.  The operators namespace is a recent addition to the library.  It was Steven Watanabe's suggestion during the pre-review to add the operators namespace, though I'd been considering it for some time.  Previously the operators were defined in the boost::polygon namespace, and to use them you either had to have boost::polygon object find them through ADL or pull the whole boost::polygon namespace in.  

Arguably, if a person is using the operators in a scope it is reasonable for them to use the using directive.

Thanks,
Luke