Le 13 févr. 08 à 18:51, Michel Pellegrin a écrit :
URL:
https://svn.lrde.epita.fr/svn/oln/trunk/milena
ChangeLog:
2008-02-13 Michel Pellegrin <pellegrin(a)lrde.epita.fr>
Commit of personnal work in my sandbox.
It's good to state what your work is about, not just that you added a
new bunch of files here.
* sandbox/pellegrin/Makefile: New.
You should be using `Makefile.am', not plain `Makefile'. Moreover,
you'll probably save time (and space).
* sandbox/pellegrin/first_test.cc: New.
* sandbox/pellegrin/set/Makefile: New.
* sandbox/pellegrin/set/multi_set.hh: New.
* sandbox/pellegrin/set/test_set.cc: New.
* sandbox/pellegrin/set/uni_set.hh: New.
* sandbox/pellegrin/set: New.
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.
Index: trunk/milena/sandbox/pellegrin/first_test.cc
===================================================================
--- trunk/milena/sandbox/pellegrin/first_test.cc (revision 0)
+++ trunk/milena/sandbox/pellegrin/first_test.cc (revision 1721)
@@ -0,0 +1,40 @@
+// Copyright (C) 2007 EPITA Research and Development Laboratory
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.
[...]
+/*! \file sandbox/pellegrin/first_test.cc
+ *
+ * \brief My first test.
+ */
+
+#include <mln/core/image2d.hh>
+
+
+int main ()
+{
+ mln::image2d<int> I;
+}
+
:)
Index:
trunk/milena/sandbox/pellegrin/set/multi_set.hh
===================================================================
--- trunk/milena/sandbox/pellegrin/set/multi_set.hh (revision 0)
+++ trunk/milena/sandbox/pellegrin/set/multi_set.hh (revision 1721)
[...]
+#ifndef __MULTI_SET_HH__
+# define __MULTI_SET_HH__
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.
+
+/*! \file sandbox/pellegrin/set/multi_set.hh
+ *
+ * \brief Definition of a point multi-set class.
+ */
« Multi-point set » seems more correct than « point multi-set ».
[...]
+ * \todo All.
This kind of comment is not really relevant. :)
+ */
+ template <typename P>
+ class multi_set : public internal::point_set_base_< P, p_set<P> >,
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.)
+ private internal::set_of_<P>
+ {
+ typedef internal::set_of_<P> super_;
+
+ public:
+ /// Forward Point_Iterator associated type.
+ typedef multi_set_fwd_piter_<P> fwd_piter;
+ /// Backward Point_Iterator associated type.
+ typedef multi_set_bkd_piter_<P> bkd_piter;
+
+ /// Constructor.
+ multi_set();
+
+ /// Test is \p p belongs to this point set.
+ bool has(const P& p) const;
+
+ /// Return the corresponding std::vector of points.
+ using super_::vect;
+
+ /// Give the number of points.
+ std::size_t npoints() const;
+
+ /// Insert a point \p p.
+ multi_set<P>& insert(const P& p);
+
+ // FIXME : doesn't compile
+ // /// Remove a point \p p.
+ // p_set<P>& remove(P& p);
+
+ /// Return the \p i-th point.
+ const P& operator[](unsigned i) const;
+
+ /// Clear this set.
+ void clear();
+
+ /// Give the exact bounding box.
+ const box_<mln_point(P)>& bbox() const;
+
+ protected:
+
+ accu::bbox<P> bb_;
+ // FIXME: Add invariant bb_.is_valid() <=> npoints() != 0
+ };
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.
[...]
Index: trunk/milena/sandbox/pellegrin/set/uni_set.hh
===================================================================
--- trunk/milena/sandbox/pellegrin/set/uni_set.hh (revision 0)
+++ trunk/milena/sandbox/pellegrin/set/uni_set.hh (revision 1721)
[...]
> +#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.
> + *
+ * \todo All.
> + */
> + 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).
Index: trunk/milena/sandbox/pellegrin/set/Makefile
===================================================================
--- trunk/milena/sandbox/pellegrin/set/Makefile (revision 0)
+++ trunk/milena/sandbox/pellegrin/set/Makefile (revision 1721)
@@ -0,0 +1,26 @@
+All_warnings = -ansi -pedantic -Wabi -Wctor-dtor-privacy -Wnon-
virtual-dtor \
+ -Wreorder -Weffc++ -Wno-deprecated -Wstrict-null-sentinel \
+ -Wno-non-template-friend -Wold-style-cast -Woverloaded-
virtual \
+ -Wno-pmf-conversions -Wsign-promo -fsyntax-only-pedantic-
errors \
+ -w -Wextra -Wall -Waddress -Waggregate-return -Wno-
attributes \
+ -Wcast-align -Wcast-qual -Wchar-subscripts -Wcomment -
Wconversion \
+ -Wno-deprecated-declarations -Wdisabled-optimization -Wno-
div-by-zero \
+ -Wno-endif-labels -Werror -Wfatal-errors -Wfloat-equal -
Wformat \
+ -Wformat=2 -Wno-format-extra-args -Wformat-nonliteral \
+ -Wformat-security -Wformat-y2k -Wimplicit -Wimport -Wno-
import \
+ -Winit-self -Winline -Wno-invalid-offsetof -Winvalid-pch \
+ -Wlarger-than-1 -Wunsafe-loop-optimizations -Wlong-long \
+ -Wmissing-braces -Wmissing-field-initializers \
+ -Wmissing-format-attribute -Wmissing-include-dirs -Wmissing-
noreturn \
+ -Wno-multichar -Wno-overflow -Woverlength-strings -Wpacked -
Wpadded \
+ -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-
type \
+ -Wsequence-point -Wshadow -Wsign-compare -Wstack-protector \
+ -Wstrict-aliasing -Wstrict-overflow -Wswitch -Wswitch-
default \
+ -Wswitch-enum -Wsystem-headers -Wtrigraphs -Wundef -
Wuninitialized \
+ -Wunknown-pragmas -Wno-pragmas -Wunreachable-code -Wunused \
+ -Wunused-function -Wunused-label -Wunused-parameter -
Wunused-value \
+ -Wunused-variable -Wvariadic-macros -Wvolatile-register-var \
+ -Wwrite-strings \
+
+all:
+ g++-4.2 -ansi -pedantic -I../.. first_test.cc -o first_test
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