Re: [Olena-patches] 875: Update and add : apply, local canvas.

Note: In order to avoid writing even more boring messages, I sometimes make remarks or give advice once in the patch, although it might apply to several spots. Don't forget to propagate the fixes throughout the whole patch! Thanks in advance. Ugo Jardonnet <ugo.jardonnet(a)lrde.epita.fr> writes: You did not mention the addition of the first mathematical morphological filters! :) (And no space before colons!) Please, don't leave empty entries. If you want to factor a description for several files, use commas (see how other ChangeLog entries do that). If a file doesn't deserve a description for the changes made to it (space adjustment, etc.), simply don't mention it in the ChangeLog. When you can, please reformat the first line of the copyright header so that it fits on a single line. Of course, if the line is larger than 80 characters, break it. Olena's header guards should not be followed by an underscore. In the current case, you should have written #ifndef OLN_FUNCTION_MIN_HH # define OLN_FUNCTION_MIN_HH Also, please don't use space to align the macro names. We do not use the same coding style as EPITA. The preferred way to declare variables is: typedef T argument; typedef void result; Optionally, you can align the names if the inferred layout looks indisputably better. We sometimes do this for virtual types declarations. But the default style is the previous one. Preferred ending guard style: #endif // ! OLN_FUNCTION_MIN_HH Otherwise, good! The spacing is quite odd here. If you want to produce beautiful comments, I suggest you have a look a rebox.el (available from my account : ~levill_r/emacs/rebox.el). Likewise, strange spacing. That seems good! There should be no space after an opening parenthesis as well as before a closing one. As for local variables, avoid aligning argument names vertically, as it's a pain to maintain. If you want to group a set of things under a common name, I suggest you use Doxygen's grouping tags instead : /// Local application on window. /// \{ // ... (enclose things here) /// \} The documentation produced by Doxygen will take them into account (you can have a look at http://www.lrde.epita.fr/~akim/compil/tc-doc/classbind_1_1BindVisitor.html to see how it looks for the Tiger Compiler). Hum, I suspect something is missing here. :) If so, add something like // FIXME: To do. within the parentheses. That's strange: is there really no diff for this file? In fact, such a function could also be applied to a window, so that comment is not exact. The directory should be named `morpho', not `morphomath'. And we use English terms as identifiers, so it should be `dilation', not `dilatation'. You can have a look a Pierre Soille's book in the library if you need the right names (https://www.lrde.epita.fr/openbiblio/shared/biblio_view.php?bibid=230&t…)pac). By the way, each time you create, rename or delete a file within Olena, you must update olena/oln/Makefile.am. Good. Don't forget to propagate the fixes. Nice patch!