22 Oct
2007
22 Oct
'07
12:30 p.m.
I'll try to review the recent patches in Olena. Please take my
comments (and Théo's, of course!) into account and fix the pointed
problems.
When the mistake is located within a ChangeLog entry, make a separate
commit, and be sure that svn-wrapper (nor Vcs) aren't involved;
otherwise, you'll end up with two ChangeLog entries. If you don't
know how to do this, ask me.
Guillaume Duhamel <guillaume.duhamel(a)lrde.epita.fr> writes:
URL: https://svn.lrde.epita.fr/svn/oln/trunk/milena
ChangeLog:
2007-10-22 Guillaume Duhamel <guillaume.duhamel(a)lrde.epita.fr>
Add a structure of tree in sandbox.
* abr.cc: New test file for mln::util::abr.
* abr.hh: New structure for tree.
Please, provide the full path to the files when documenting
ChangeLogs!
The simplest way to get things done right is to perform `svn commit'
from the `milena' directory. Both svn-wrapper and Vcs will provide
you with a pre-filled ChangeLog entry.
Index: trunk/milena/sandbox/duhamel/abr.hh
===================================================================
--- trunk/milena/sandbox/duhamel/abr.hh (revision 0)
+++ trunk/milena/sandbox/duhamel/abr.hh (revision 1362)
@@ -0,0 +1,158 @@
+// Copyright (C) 2007 EPITA Research and Development Laboratory
+//
+// This file is part of the Olena Library. This library is free
+// software; you can redistribute it and/or modify it under the terms
+// of the GNU General Public License version 2 as published by the
+// Free Software Foundation.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this library; see the file COPYING. If not, write to
+// the Free Software Foundation, 51 Franklin Street, Fifth Floor,
+// Boston, MA 02111-1307, USA.
+//
+// As a special exception, you may use this file as part of a free
+// software library without restriction. Specifically, if other files
+// instantiate templates or use macros or inline functions from this
+// file, or you compile this file and link it with other files to
+// produce an executable, this file does not by itself cause the
+// resulting executable to be covered by the GNU General Public
+// License.
+// reasons why the executable file might be covered by the GNU General
+// Public License.
There's a glitch here. Please pay attention when using copy and paste.
+#ifndef MLN_UTIL_ABR_HH
+# define MLN_UTIL_ABR_HH
+
+# include <list>
+# include <iostream>
`iostream' shall not be included by any Milena file (except, maybe, in
some files of the I/O subsystem)! Please remove this #include
directive.
+/*!
+ * \file mln/util/abr.hh
+ *
+ * \brief Definition of a generic general tree.
+ *
+ *
+ */
Please give a definition of `ABR' in the documentation.
+namespace mln
+{
+
+ namespace util
+ {
+
+ template <typename T>
+ struct s_abr
We're not at EPITA here: the coding style from Ing 1 does not apply to
LRDE projects! So don't prepend a `s_' to structures nor to classes.
+ {
+ s_abr ();
+ s_abr (const T& elt);
+
+ void add_son (const T& elt);
The canonical term is `child', not `son' (even if we use the word
« fils » in French).
+ void print_rec (int n) const;
+ void print (void) const;
+ int search_rec(s_abr<T>** res, const T& elt);
+ s_abr<T>* search(const T& elt);
+
+ const T& elt_;
Likewise (with respect to the epitean coding style): please don't
systematically align the names of declared variables with tabs.
+ s_abr<T>* father_;
I think `parent_' would be a better name than `father_'. Check this
with Théo.
+ std::list< s_abr<T>* > sons_;
+ };
+
+
+# ifndef MLN_INCLUDE_ONLY
+
+ template <typename T>
+ s_abr<T>::s_abr ()
+ : elt_ (0),
+ father_ (0)
+ {
+ }
There seems to be a problem with the indentation here : the `f' of
`father_' should be just below the `e' of `elt_'. Likewise for the
following ctor definition.
+
+ template <typename T>
+ s_abr<T>::s_abr (const T& elt)
+ : elt_ (elt),
+ father_ (0)
+ {
+ }
+
+
+ template <typename T>
+ void
+ s_abr<T>::add_son (const T& elt)
+ {
+
+ s_abr<T>* s = new s_abr<T> (elt);
+
+ s->father_ = this;
+ this->sons_.push_back (s);
+ }
+
+ template <typename T>
+ void
+ s_abr<T>::print_rec (int n) const
+ {
+ std::cout << this->elt_ << std::endl;
The stream shall be passed as an argument, and not hard-coded in the
body of the method!
+ typename std::list<s_abr<T>*
>::const_iterator it = this->sons_.begin ();
+ for (; it != this->sons_.end (); ++it)
Why don't you define `it' within the for-loop?
+ {
+ for (int i = 0; i < n; ++i)
+ std::cout << " ";
+ (**it).print_rec (n + 1);
+ }
+ }
I think we should get rid of this kind of recursive functions: a C++
compiler is not as clever as a compiler of a functional language, and
cannot fully optimize recursive calls (or, more precisely, the
language does not provide enough semantics to allow it, and it's a
best effort from the compiler). Think about inlining, for instance.
+ template <typename T>
+ void
+ s_abr<T>::print (void) const
+ {
+ this->print_rec(1);
+ }
Please use a consistent coding style: either add a space before
opening parenthesis of a function definition/call uniformly, or don't.
But do not mix the styles. As far as I am concerned, both are OK,
even if the coding style of Olena 0.11 recommended to add no space
before a `('.
+ template <typename T>
+ int
+ s_abr<T>::search_rec(s_abr<T>** res, const T& elt)
+ {
+ if (elt == this->elt_)
+ {
+ *res = this;
+ return 1;
+ }
+ else
+ {
+ typename std::list<s_abr<T>* >::iterator it = this->sons_.begin ();
+ for (; it != this->sons_.end (); ++it)
Likewise: prefer defining iterator *within* the loop, rather than
outside. It's better from both the semantic (the iterator is
``attached to the loop'') and the safety (the scope of the iterator is
limited the loop) points of view.
+ {
+ if ((**it).search_rec (res, elt))
+ return 1;
+ }
+ }
+ return 0;
+ }
+
+ template <typename T>
+ s_abr<T>*
+ s_abr<T>::search(const T& elt)
+ {
+ s_abr<T>* res = 0;
+
+ if (search_rec (&res, elt))
+ return res;
+ std::cerr << elt << " not found" << std::endl;
To my mind, this function should not perform such a side effect. And
even in that case, as above, it shall not hard-code the target stream.
+ return 0;
+ }
+
+# endif // ! MLN_INCLUDE_ONLY
+
+ } // end of namespace mln::util
+
+
+} // end of namespace mln
+
+
+#endif // !MLN_UTIL_ABR_HH
Index: trunk/milena/sandbox/duhamel/abr.cc
===================================================================
--- trunk/milena/sandbox/duhamel/abr.cc (revision 0)
+++ trunk/milena/sandbox/duhamel/abr.cc (revision 1362)
@@ -0,0 +1,49 @@
+// Copyright (C) 2007 EPITA Research and Development Laboratory
+//
+// This file is part of the Olena Library. This library is free
+// software; you can redistribute it and/or modify it under the terms
+// of the GNU General Public License version 2 as published by the
+// Free Software Foundation.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this library; see the file COPYING. If not, write to
+// the Free Software Foundation, 51 Franklin Street, Fifth Floor,
+// Boston, MA 02111-1307, USA.
+//
+// As a special exception, you may use this file as part of a free
+// software library without restriction. Specifically, if other files
+// instantiate templates or use macros or inline functions from this
+// file, or you compile this file and link it with other files to
+// produce an executable, this file does not by itself cause the
+// resulting executable to be covered by the GNU General Public
+// License.
+// reasons why the executable file might be covered by the GNU General
+// Public License.
Likewise, glitch. We have a saying here at the LRDE which states:
« Le copier-coller TUE le programmeur. »
which means: copy-and-paste are of uttermost usefulness, but still,
take care of the result!
A piece of advice: proofread your patch before sending it.
+/*!
+ * \file tests/abr.cc
+ *
+ * \brief test of mln::util::abr
+ *
+ */
+
+#include "abr.hh"
+
+int main (void)
+{
+ using namespace mln;
+ util::s_abr<unsigned> abr (1);
+
+ abr.add_son (2);
+ abr.add_son (3);
+ abr.print ();
+ util::s_abr<unsigned>* abr2 = abr.search (2);
+ abr2->add_son (4);
+ abr2->add_son (5);
+ abr.print ();
+}
Ok. Maybe one or two assertions would be fine.
BTW, why is this code located in milena/sandbox/duhamel? Did Théo
require this?