Re: [boost] Proposal/InterestCheck: Boost.Geom

Subject: Re: [boost] Proposal/InterestCheck: Boost.Geom
From: Simonson, Lucanus J (lucanus.j.simonson_at_[hidden])
Date: 2009-01-29 13:28:04

Anis Benyelloul wrote:
> Barend Gehrels <barend <at> geodan.nl> writes:
> 
>> If you're building a library piece you
>> probably want the indirect approach. If you've a piece of user code,
>> you probably find the direct approach more readable.
> 
> My purpose is to make the indirect approach as/more readable and
> convenient as the direct one so that the user's won't have to tie
> their code to the underlying framework's points and boxes (i.e they
> won't have to deal directly with QPoints at all).

We have to use the indirect approach to implement our library algorithms.  We would like doing so to be as readable and convenient as possible so we aren't writing unreadable, inconvenient code.

> It seems that your purpose is rather: Provide lots of algorithms and
> make them applicable to any type the user is currently working with
> (the user will still have to know/use QPoints before passing them to
> the algotithms). 
> 
> Or maybe I missed something?

Our goal is to do both.  We want people to write their own generic algorithms instead of tying them to frameworks and legacy type systems, and we also want to provide high quality algorithms for generally useful operations that people shouldn't have to write for themselves.

Your rectangle API is the following (comments added by me for discussion purposes):

//does not initialize, depends on wrapped object's default constructor, which may not exist btw, and may not initialize
//if you (or a user) inadvertently write code that depends on the unitialized value it will behave differently with different wrapped T types
box()

//there is a static assert in operator= so this is type checked which is good
template<class Impl>
box(const Impl& im){ this->operator=(im); }
        
box(const box& im);
        
//casts this to wrapped T
BoxImpl& impl();

//casts this to const wrapped T
const BoxImpl& impl() const;

//your use of macros to define getters and setters saves you typing, but hurts readability
get_* and set_* where star can be any x1, y1, z1, x2, y2, z2, xmin, ymin, zmin, xmax, ymax, zmax, width, height, depth

//why do you provide get/set when you have the property acessors as well
//isn't providing one or the other sufficient (more minimal) and better than providing both?
Macro definitions of property accessors: bx.width()=bx.height()+3 value=bx.xmax()*bx.xmin() ... etc		

//I don't know what this does, are you assuming rectangles can be empty?
bool valid() const;

//you have static assert that Box specializes box_traits	
template<class Box>
box& operator=(const Box& bx);

box& operator=(const box& bx);

//I'm guessing that this adds p.x to x1 and x2 and p.y to y1 and y2, but it could just as easily
//add p.x to xmax and subtract p.x from xmin etc.
template<class Point>
box<typename traits_type::box_type>
operator+(const Point& p) const;

template<class Point>
box<typename traits_type::box_type>
operator-(const Point& p) const;

//I'm guessing this clips the one box to the other...	
template<class Box>
box<typename detail::box_ret_type<BoxImpl, Box>::type>
operator&(const Box& bx) const;

//I can't imagine what this does, perhaps grow this box to encompass bx?
template<class Box>
box<typename detail::box_ret_type<BoxImpl, Box>::type>
operator|(const Box& bx) const;
        
//you have static assert
template<class Box>
bool operator==(const Box& bx) const;
template<class Box>
bool operator!=(const Box& bx) const;
template<class Point>
box& operator+=(const Point& pt);
template<class Point>
box& operator-=(const Point& pt);
template<class Box>
box& operator&=(const Box& b);
template<class Box>
box& operator|=(const Box& b);

///////
// Corner accessors
template<x_corner xc, y_corner yc, z_corner zc>
point<typename detail::corner_wrapper<impl_type, xc, yc, zc>::type>&
corner();
template<x_corner xc, y_corner yc, z_corner zc>
const point<typename detail::corner_wrapper<impl_type, xc, yc, zc>::type>&
corner() const;

/// you could have saved yourself typing by putting
template<x_corner xc, y_corner yc, z_corner zc = z_corner_dummy> // above instead of repeating:
template<x_corner xc, y_corner yc>
point<typename detail::corner_wrapper<impl_type,xc,yc,z_corner_dummy>::type>&
corner();
template<x_corner xc, y_corner yc>
const	point<typename detail::corner_wrapper<impl_type,xc,yc,z_corner_dummy>::type>&
corner() const;

////
// Center
center_wrapper& center();
const center_wrapper& center() const;

I wasn't able to find the static function you mentioned that casts BoxImpl to box<BoxImpl>, perhaps you meant that it auto casts by value through the constructor that takes T?

What if you want to have a operator+ between two rectangles?  Lets say it adds xmin to xmin, xmax to xmax etc, how would you do it without ambiguity with operator+ between rectangle and point?
If you look in my vault gtl/RectangleImpl.h you will find that I defined quite a few more functions on my rectangle wrapper than you do on box.  Imagine I were a user of your library and I wanted all these functions, how would I extend your library?  Would I modify your source file to add their definitions to your box wrapper?  What happens to me then when you release a new version of your library?  What if I wanted to define my own operator| between any two rectangles, but it conflicts with yours, causing compile time or runtime errors where sometimes yours is called, sometimes mine, depending on whether the lhs is box<T> or not.  I'd like to have the option of not including your operator functions, but still use your getters and setters (perhaps to implement by own operator|, for example.)  Great, now I have to add isolating my own generic operators to my todo list....

Particularly when writing generic code (but for C++ in general) we prefer to implement functions as non-member non-friend and provide a minimal and sufficient interface on a class to allow it to encapsulate its invariant.  Does box maintain the invariant that xmin <= xmax?  If I set xmin to something greater than xmax, what happens, btw?  From this viewpoint, these getters and setters should be the only interface to the class other than the constructors/assignment operators.  I believe someone provided me a link to Sutter or someone comparable's article that discusses this, and Bjarne also talks about this issue when he gives his talks.  Again, until you've experienced the problems this solves it might not make much sense.  Recall that I'm telling you this but also originally did exactly the same thing you did only more so.  I changed my mind about this based on more than just the say-so of the people who told me the same thing I'm telling you now.

Regards,
Luke