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-15 14:49:35

Fernando Cacciola wrote:
>> 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)

I'm fully committed to supporting all mainstream compilers (including adding workarounds for broken compilers when possible) as well as resolving issues people run into with ADL such as replacing all instances of set() with (set)() and fixing warnings in all mainstream compilers.  I've fixed all instances of quite a few of the effective C++ warnings not that long ago and put a lot of work into making the code compile warning free in several versions of gcc and edg.  I would plan on fixing all MSVC8 and 9 warnings before ever letting the library into boost because I don't want to read all the complaints about warnings anymore than the users want to read the warnings.

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

I really like the current name, but I suppose I can change it if needed.
 
I'd like suggestions for names from the community.  Its easy to shoot down a name, but not so easy to come up with a name that won't be shot down.  I'd prefer a name that is descriptive to help people know what to expect of the library.

Here's the result of my brainstorming:
Boost.Shape
Boost.Shapeshifter
Boost.ipcg (Integer Planar Cartesion Geometry)

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

The ones that are wrapped by an encapsulating class are named <classname>_enum, where as the ones where it is the enum type itself that is intended for use is named without the _enum.  Originally the class encapsulated enums where implemented with the enums as private member types and the constants as static const declarations of the class type.  This however resulted in those constants being memory locations and runtime variables everwhere the compiler should have evaluated them at compile time because of the lack of constexpr.  I changed the constants to enum values to force the compilers to do the right thing and treat them as constants.  This forced me to move the enum declarations out of the classes, which is how we got to where we are today.  I would prefer to have constexpr.

>> 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.

 template <typename geometry_type, typename scale_factor_type>
 enable_if<..., void>::type scale(geometry_type &obj, scale_factor_type numerator,
                                  scale_factor_type denominator = 1);

Scale_factor_type can be integer or floating point and we can scale down by integer factor of eg. 10 with scale(1, 10) without the need to convert 1/10 to floating point.  I'd be very dissapointed in any compiler that fails to compile away the (value / 1) division identity property sub-expression tree.

I don't think that the isotropic_scale_factor is currently used.  It was part of the original design which turned out to be not very needed by my internal user base.

Thanks,
Luke