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-09-14 13:13:31

Fernando Cacciola wrote:
> (*)
> I really don't like the use of overloading-on-concept as the main API
> element. Specially implemented via SFINAE as it is.
> 
> IMO, a much better design would be to a have basic traits that
> provides the fundamental building blocks, as the library is doing,
> for example via "point_traits<>", but then the derived functions be
> also provided as static 
> members of a template class, say, point_concept<>
> 
> This is much better for a number of reasons:
> 
> A user can specialize the functions for their own data types in a
> snap. 
> Currently is next to impossible. For instance, having the compiler
> call my own assign() for my own point type, when it happens to be a
> model of  a mutable 
> point, can be very very tricky.
> 
> A design based on many layers of specialzable classes is WAY more
> compiler 
> friendly and hence much more easy for the programmer to spot mistakes
> such as constrains violations (wrong type) without concept checking.
> 
> Then, at the front end of the API, I would provide the overloaded
> versions as 
> there are now as a top layer. This is in fact what the library is
> doing for 
> those fundamental functions that are given directly by the traits
> class, but IMO 
> it needs to do the same with all free functions, dispatching the
> actual implementation to the right specialization of the concept
> (i.e. free fucntions would be merely forwarding)
> 
> Finally, the library itself wouldn't use the overloaded free
> functions but the "real" member functions. This would make the code
> much more robust in the 
> presence of ADL. Right now, there are countless places where
> unqualified calls 
> to, say, assign(), set(), even "abs()", WILL cause ambiguities in
> many real 
> world contexts.

This suggesting is similar to the previous API design.  I found it was challenging to do so much typing.  I agree that it would help with ADL problems, and it was my policy to use the internal functions when they were still part of the design.  My goal was to show how a C++ concepts based API would look and feel in a real world C++ library that people can use now.  I think that as a case study for C++ concepts Boost.Polygon is a very interesting piece of software with an important place in the ongoing discussion of C++ concept and for that reason it is important for it to be prominently exposed to the C++ community as a boost library.  That is the intention of the design and submission to boost.

> (*)
> In fact, that most opertations modify their arguments instead of
> producing new 
> ones is odd, and in some cases just plain wrong (when the reference
> parameter is not even an input value)

Can you elaborate?  I don't know which functions you are referring to here.
 
> (*)
> The library is specifically 2D yet that fact isn't encoded in the
> namespace nor 
> the identifies. This should be fixed as otherwise will conflict with
> future 3D libraries.

You foresee a different boost.polygon namespace?
 
> (*)
> The library names something as "orientation". IMNSHO, in the context
> of 
> polygos, orientation means something else (that which the library
> call direction instead). So, I would strongly suggest the rename it,
> to something like: axis_alignment (which is what it is).

I would prefer to keep the names the same.  I don't believe people who are used to thinking of winding direction as orientation will be confused for long.  Most users don't really understand winding at all, and power users who might think of it as orientation aren't the type who will be easily confused.

> (*)
> some enums are named after "BLABLA_enum" and some after "BLABLA".
> that should be consistent... it would remove _enum

Those enums are usually wrapped by an encapsulating class.  Their names are meant for internal use.  The values are meant for users, and the names of the enum values tend to be straightforward.

> (*)
> 
> I don't think it makes sense to name "point_data<>" with the "_data"
> postfix. I understand "_traits" etc... but point should be just point
> IMO. 

I agree with you.  I chose to be extra explicit so that people could not become confused about the different between a data type and what is a concept, which is easy to happen since concepts are a new style element to most users.

> (*)
> 
> I dislike that there is scale and move not being merely convenience
> warppers of transform() (also there).
> 
> And I *really* dislike the existence of a scale_up() + scale_down()
> which are 
> BOTH there simply because they take "integer" factors instead of a
> fraction (whether a rational number or a floating point).
> Also, scale down *secretly* uses a "rouding_policy", instead of
> taking it as argument.
> 
> IMO, there should be just scale(), which to add confusion already is
> but is 
> really an "applicator" that takes a an scaling function object doing
> whatever 
> and applying thee point's coordinate into it. Which is the same
> transform() does. 
> 
> But, anyway, IMO there should be just one "scale" taking a floating
> point 
> factor, or if you prefer, a rational number (expressed as separate
> numerator and denominator integer arguments for instance).  The
> rounding policy would be given 
> as optional aregument.
> 
> 
> There is also the transformation<> (along with the transform()
> function), and 
> the isotropic_scale_factor<>, which are naturally related to these
> scaling operators. IMO, these is overegineered and leaves the user
> with too many choices. 

Originally there was a simpler design which implemented only isotropic_scale_factor based scaling, but it grew with additional requests from users to add scaling by integer and scaling by floating point.  You are seeing the result of a legacy of internal use of the library.

> (*)
> Likewise, polygon.hpp should be renamed since it is really the mother
> header of 
> the library, including all others. I can imagine that it is called
> "polygon.hpp" since that's the name of the library which I expected
> to find on it stuff specifically related to a polygon data type or
> concept or such. 

What do you suggest polygon.hpp be named?  
 
> (*)
> 
> Many classes have "inline" in the method definitions. This is
> completely 
> unnecesary since in-class definitions are inline already. IMO this
> needs to be removed.
 
This happened primarily because functions that were free functions in name spaces moved to static member functions of templated structs and I neglected to remove the irrellevant inline declarations.  If it is in the implementation details does it really matter?
 
> (*)
> 
> There are some structs wrapping enums to make them type safe. That's
> useful, but then the structs store the enum value as int (or unsigned
> int depending on where you look), while a char would be enough.

This is true.  I can likely change them to char without needing to change anything else and not break the functionality of the library.

>> - What is your evaluation of the documentation?
> 
> As many others said it really needs a good introduction that a
> newcomer can read 
> to see what the library is, and perhaps more importantly, is NOT,
> about. 
> 
> Also some tutorials, both on using the library out of the box with
> its own 
> models and on using it on user-defined types.
> 
> And as some reviwers said, the documentation is way too focus on
> concepts and 
> its needs to document models as well. In fact, I think most "junior"
> users won't 
> be interested at all in adapating their own types but on just using
> the library with the types provided, so the documentation for the
> models should come first, 
> and the concepts should go after in the chapter discussing adaptation.

This sounds like a good suggestion.

Thanks,
Luke