Re: [boost] [Polygon] review - isotropy.hpp

Subject: Re: [boost] [Polygon] review - isotropy.hpp
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2009-08-29 17:29:08

AMDG

All comments are based on revision 55787.

isotropy.hpp:overall
There seems to be a fair amount of duplication with
boost::enable_if, requires, and is_same_SFINAE.

isotropy.hpp:194
I think that all arithmetic types should be supported
either through enumerating all specializations or by
explicit metaprogramming:
template<class T>
struct geometry_concept
 : mpl::if_<is_arithmetic<T>
     , coordinate_concept
     , undefined_concept
   >
{};
Also, it would be nice to be able to use a nested typedef
instead of specializing geometry_traits.  (Think of the
way std::iterator_traits works.)  This works better with
inheritance, since nested typedefs are inherited, but
specializations are not.  Since you seem to want the
absence of any specialization to be detectable, it would
have to be implemented something like:
BOOST_MPL_HAS_XXX_TRAIT_DEF(geometry_concept_type);
template<class T>
struct get_geometry_concept_type {
    typedef typename T::geometry_concept_type type;
};
template<class T>
struct geometry_concept
  : mpl::eval_if<has_geometry_concept_type<T>
      , get_geometry_concept_type<T>
      , mpl::identity<undefined_concept>
    >
{};

isotropy.hpp:205
requires is too generic a name.  It should be
something like enable_if_same.

isotropy.hpp:205
This #ifdef is a little odd.  I'm assuming that
SFINAE is not strictly needed in gtl_if, but it
allows extra short-circuiting when possible? 
In addition, since the problems is specific to
msvc, not to win32, the test should be
#if BOOST_WORKAROUND(BOOST_MSVC, BOOST_TESTED_AT(1500))
(BOOST_MSVC exists because detecing msvc accurately
in the preprocessor is a pain, if I remember correctly)

isotropy.hpp:308
What does y_c_edist mean, and why are you including a
constant in the gtl_and for euclidean_distance?

isotropy.hpp:328
s/garenteed/guaranteed/

isotropy.hpp:385
This seems excessively clever compared to a condition.

isotropy.hpp:464-470
These functions need no be declared inline or they will
cause link errors when included in more than one translation
unit.

isotropy.hpp:464
So, WEST and SOUTH become LOW and
NORTH and EAST become HIGH?  Is this
conversion really a good idea?

isotropy.hpp:528
UP and DOWN are missing from this list.

isotropy.hpp:535-541
More functions that need to be inline.

In Christ,
Steven Watanabe