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@lrde.epita.fr> writes:
URL: https://svn.lrde.epita.fr/svn/oln/trunk/milena
ChangeLog: 2007-10-22 Guillaume Duhamel <guillaume.duhamel@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?