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

Subject: Re: [boost] [Polygon] review - isotropy.hpp
From: Simonson, Lucanus J (lucanus.j.simonson_at_[hidden])
Date: 2009-08-29 18:58:44

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

requires and is_smae_SFINAE are no longer used and will be removed.

> 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>
>     >
> {};

We will have to think more about this.  It may not always be true that we want subtypes to inherit concept types, though it should be if the subtyping relationship is valid.  I think I experimented with a member tag for geometry type, but found that a default definition for the metafunction for looking up the geometry concept interfered with the way I was doing SFINAE at the time.  I'll have to think more about it and experiment a little.  I think it may be a good idea.

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

requires will be removed.

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

You are correct, gtl_if is used to provide short circuit SFINAE behavior in non-msvc compilers.  Specifically it works around compiler bugs and prevents parsing of templates that produce syntax errors under circumstances where SFINAE is intended to suppress instantiation.

You are right that win32 is the wrong thing.  I need it to be functional in the BOOST_POLYGON_NO_DEPS standalone mode, however, so I need to know how BOOST_MSVC works so that I can replicate it in standalone compilation.

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

This is a compiler workaround for MSVC8.  There are a large number of them, unfortuantely.  It is named with a mangling scheme related to the concept and function name that uses it.  MSVC8 has two different code paths for instantiating templates and applying SFINAE.  The one where it recognizes that it has seen a subtemplate before is incorrect and I used these constant types that are unique to each function to force MSVC8 to SFINAE correctly.  I could perhaps put a

 //MSVC8 workaround 

on each one so that it doesn't cause too much confusion.

> isotropy.hpp:328
> s/garenteed/guaranteed/

fixed

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

bool would get the job done, true.  orientation_2d is typesafe and provides a set of utilities.  It becomes very important in the definition of the rectangle algorithms in particular.

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

These are the definitions, at the declarations they are declared inline.

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

This is the defintion of a constructor declared with explicit above.  The conversion is useful.

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

fixed

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

More definitions of functions that were declared inline above.  I guess I left off explicit and inline on these because once at the declaration is enough and I was trying to pack them into one line.

Thanks for the careful reading. 
Luke