Re: [Olena-patches] milena r1721: Commit of personnal work in my sandbox

Le 13 févr. 08 à 18:51, Michel Pellegrin a écrit : It's good to state what your work is about, not just that you added a new bunch of files here. You should be using `Makefile.am', not plain `Makefile'. Moreover, you'll probably save time (and space). Mentioning new directories is optional, IMHO. But if you do so, it'd be more logical to make them appear /before/ the files they contain. :) Remember, a ChangeLog entry should be readable top-down, most of the time. It's of very little matter (because we can automate this later), but it's still better to have the right years in the copyright headers (at least because those files are exposed through the repository to the rest of the world). So please pay attention when copying and pasting files or part of files. Likewise in other files. [...] :) [...] Milena's headers use a standard format (e.g., MLN_LEVEL_STRETCH_HH for mln/level/stretch.hh). Please try to stick to these conventions, as it will ease the transition from your sandbox to Milena's tree. « Multi-point set » seems more correct than « point multi-set ». [...] This kind of comment is not really relevant. :) Careful, the second parameter of internal::point_set_base_ must be the exact type of the object! (I.e., multi_set<P>). (But you might have changed this in later patches.) I haven't read the following patches yet, but you probably lack a set of definitions of properties (``traits'') on this point set here. And it's strange not to see a std::multiset as attribute, too. [...] [...] > +#ifndef __UNI_SET_HH__ > +# define __UNI_SET_HH__ > + > +/*! \file sandbox/pellegrin/set/uni_set.hh > + * > + * \brief Definition of a point uni-set class. > + */ > + > +# include <mln/core/internal/point_set_base.hh> > +# include <mln/core/internal/set_of.hh> > + > + > +namespace mln > +{ > + > + /*! \brief Point set class. > + * > + * This is a mathematical uni-set of points (not a multi-set). The > + * parameter \p P shall be a Point type. > + * > + */ > + template <typename P> > + class uni_set : public internal::point_set_base_< P, p_set<P> >, > + private internal::set_of_<P> Hum... What's the purpose of this class? I can see two answers: - a concrete uni-set based on std::set; but we already have that (as p_set<P>) - an abstract class for uni-sets (but in that case, it should take an exact type as paramater, which is not the case). You should have a look at how we write Makefiles.am in the subdirectories of tests/, and use that instead of hard-coded rules. [...] Thanks for this first patch! Some of my remarks may be absolutely out- of-date (since you might have changed a lot of things in your following patches). In that case, just ignore them, of course. -- Roland