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

Subject: Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009
From: Fernando Cacciola (fernando.cacciola_at_[hidden])
Date: 2009-09-15 13:47:54

Luke,

> 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.
> 
Fair enough.
As I said I think the current design is a stretch of current C++ but I can live 
with that, *provided* that you do the hard work of making it usable with 
mainstream compilers (i.e. adding workarounds, resolving ADL issues, fixing 
warnings, etc)

>> (*) 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.
> 
center() for example.

>> (*) 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?

It is easy to forsee someone wanting to integrate 3D polygons.
Where would that be?
It can't be merged within "boost::polygon" because it will most likely clash 
with your names, unless you names are made much less general (i.e. planar specific).
So it will end up in say "boost::polygon_3d" or such.

But then there would be an unfair asymmetry.

This is actually a general issue with the library name, not just it's namespace:
"Polygon" is too a broad a name... there can be in 2D, 3D, snapped to integer 
grids, or the floating point cartesian plane, they can have homogenerous 
coordinates, etc.. etc...

So, like many others said, you need to come up with a more specific name for the 
library. IMO, it should clearly express the two central constaints: 2D and 
integer coordinates.
Have in mind that you *can* come up with something creative. Consider Spirit and 
Fusion for instance.

> 
>> (*) 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.
> 
Fair enough.

> 
>> (*) 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.
> 
Does that mean that they shouln't be named consistently?
They are not even in a "detail" namespace, so it's not even evident that they 
are for internal use only (and even then they could be named consistently)

>> (*)
>> 
>> 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.

Right, but as in most cases drawing the disctinction in one of the confsuing 
elements is enough.

> 
>> (*)
>> 
>> 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.
> 
OK, and what do you propose for Boost now?
The current legacy design seems unnecesarilty confusing.

>> (*) 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?
> 
The same as the library, whic has I said above I don't think it should be named 
"Polygon"

>> (*)
>> 
>> 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?
> 
To some extent it does. Boost requires certain quality of implementation, and 
measured by several factors, including style. Now again, it could be my own 
subjective measure that redundant unncesesary elements are bad style (just like 
foo (void) for example)

Of course this isn't something to digress on, but IMO it would improve the QoI 
if you remove redundant elements.