Difference between revisions of "Janitorial tasks"

From Inkscape Wiki
Jump to navigation Jump to search
m (moved InkscapeJanitors to Janitorial tasks: kill wikiwords)
Line 1: Line 1:
This is a quick list of tasks needing to be done, that no one has gotten around to yet.  They're not in any order yet, and simply have a note at the end with who added them to the list, so if you have questions, you can ask them.  (Or, ask the devel mailing list.) See also [[DeveloperManual]].
+
Before embarking on any of those tasks, be sure to contact the developers on the mailing list.
  
 
+
== Inline function removal ==
 
 
== Inline Function Removal ==
 
 
* In src/xml/repr.h, there are a bunch of inline functions that are trivial wrappers for SPRepr member functions.  Your task, should you choose to accept it:  replace all calls to those wrapper functions with direct calls to the wrapped member functions, then remove the unused wrapper functions. (mental) (some patches have been applied from Robert Crosbie)
 
* In src/xml/repr.h, there are a bunch of inline functions that are trivial wrappers for SPRepr member functions.  Your task, should you choose to accept it:  replace all calls to those wrapper functions with direct calls to the wrapped member functions, then remove the unused wrapper functions. (mental) (some patches have been applied from Robert Crosbie)
 
** mental says don't touch sp_repr_parent and sp_repr_next (mentioned sp_repr_prev, but it doesn't exist)
 
** mental says don't touch sp_repr_parent and sp_repr_next (mentioned sp_repr_prev, but it doesn't exist)
Line 13: Line 11:
 
* There is a python script that can help for this kind of replacements. Have a look at it: [[ReplacementScript]]
 
* There is a python script that can help for this kind of replacements. Have a look at it: [[ReplacementScript]]
  
== Class Design Consistency ==
+
== Improving headers for compilation speed ==
* Adding default, private, un-defined copy ctor and operator = . As a rule, all C++ classes should have an explicit copy constructor and assignment operator (operator =). Instead of defaulting to the standard one, any classes that don't already have them should have the two declared in the "private:" section of the class, but never actually have those functions defined. ([[JonCruz]])
 
<pre>
 
class Foo {
 
  ...
 
private:
 
    Foo(const Foo& other); // no copy
 
    void operator=(const Foo& other); // no assign
 
};
 
</pre>
 
 
 
* When a class has a destructor, always make it virtual. Just to be on the safe side. See [http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7].
 
 
 
<pre>
 
class Foo {
 
...
 
    virtual ~Foo();
 
...
 
};
 
</pre>
 
 
 
Question: when the class does not have a dtor, but does have virtual methods; should virtual ~Foo() {}; be added? (perhaps derived classes do have destructors...)
 
 
 
== Add Assertions ==
 
* Look for functions that are using pointer variables without checking against NULL first.  Add a call to <code>g_assert( )</code>, <code>g_return_if_fail( )</code>, etc.:
 
 
 
void inkscape_dispose(GObject * obj) {
 
    Inkscape::Application *inkscape = (Inkscape::Application *) object;
 
    g_assert(inkscape != NULL);  // <-- add this assertion
 
 
    while (inkscape->documents) {
 
        /* ... */
 
    }
 
}
 
 
 
(BPF) This could be contentious, and <code>g_assert( )</code> will nearly always be the wrong thing here. I would not recommend adding such assertions to
 
apparently good code, but it is certainly fair to assert before dereferencing a pointer in new code, with a view to removing the assertion once
 
the code is de-bugged. I may have become confused because if the function in question is known to (or better, documented to) require a non-NULL
 
pointer input then there should be a PRECONDITION macro at the head of the function which must provide correct run-time and release time behaviour.
 
This would be the case even if the first use of the pointer is hundreds of lines into the function. See Bug [ 1210100 ] "Selecting corrupted embedded images crashes".
 
 
 
== Improving Headers for Compilation Speed ==
 
 
When a header file changes, any file that includes it is recompiled.  This becomes a problem when a given header file is included by other header files, as it leads to a vast header include tree, wherein a small change to a seemingly minor header file causes massive rebuilds of much of the codebase.   
 
When a header file changes, any file that includes it is recompiled.  This becomes a problem when a given header file is included by other header files, as it leads to a vast header include tree, wherein a small change to a seemingly minor header file causes massive rebuilds of much of the codebase.   
  
 
Fortunately for us, there are recognised techniques to mitigate this!  Here's what to do:
 
Fortunately for us, there are recognised techniques to mitigate this!  Here's what to do:
  
* Find a header that includes several other headers.  For example, the header "bar.h" might have a <code>class Bar</code> with a member of type <code>Foo*</code>, thus it includes "foo.h" to get the definition of that type.  You can prune this by using a 'forward declaration', whenever the <code>Foo</code> entity is not itself used, only pointers to it (as stated):
+
* Find a header that includes several other headers.  For example, the header "bar.h" might have a <code>class Bar</code> with a member of type <code>Foo*</code>, thus it includes "foo.h" to get the definition of that type.  You can prune this by using a 'forward declaration', whenever the <code>Foo</code> entity is not itself used, only pointers and/or references to it:
  
 
  //#include "foo.h"  /* <-- kill the header include! */
 
  //#include "foo.h"  /* <-- kill the header include! */
Line 74: Line 31:
 
(BPF) The [http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.11 C++ FAQ Sheet] explains how to code forward declarations (classes that both need to know about each other); To fully understand why, you should study the [http://www.gotw.ca/publications/mill04.htm Pimpl idiom].
 
(BPF) The [http://www.parashift.com/c++-faq-lite/misc-technical-issues.html#faq-39.11 C++ FAQ Sheet] explains how to code forward declarations (classes that both need to know about each other); To fully understand why, you should study the [http://www.gotw.ca/publications/mill04.htm Pimpl idiom].
  
== Cleanup: Whitespace ==
+
== Source formatting ==
* Tabs in the source just lead to many troubles, so they aren't supposed to be used. One problem is that just converting them to spaces introduces extra diffs in the CVS history, so someone wanting to remove tabs should first figure out where they are and then talk over an approach for cleansing them. ([[JonCruz]])
+
* Source files should use four spaces as indentation and no tabs. Trailing whitespace should be removed.
 
+
* The comment at the top of each file should have the following format. Note that the Doxygen <tt>@file</tt> comment does not include the author information, which is in a regular multiline comment. Author emails can be obfuscated, but should be real addresses.
* Trailing whitespace is also a non-visible but diff-confusing issue, and should also eventually tracked down and removed. ([[JonCruz]])
+
/** @file
::See also http://permalink.gmane.org/gmane.comp.graphics.inkscape.devel/21241 which has a script for removing trailing whitespace. The thread talks about cleaning these when the area is touched anyway, but is against doing a full cleanup.
+
  * @brief Logarithmic time traveling salesman solver - public interface
 
+
  *//*
== Cleanup: Syntactical ==
+
  * Authors:
* Make sure all files include the standard copyright/license info -- be careful though!  Check with ALL the listed copyright holders before replacing a license header.  If there is no header, track down the authors and get permission before. Note that a copyright notice to "The Inkscape Organization" is not valid; you will need to track down the original authors in that case too.
+
  *  J. God Hacker <ihatepizza@gurus.org>
 
+
  *  Ellen Epic <epicwin at email dot com>
== Cleanup: Modelines ==
+
  *
* Make sure all files include the emacs [http://www.delorie.com/gnu/docs/emacs/cc-mode_26.html Local Variable block] and a vim [http://www.vim.org/tips/tip.php?tip_id=331 modeline] at the end of the file. There is an example modeline on the [[Coding_Style]] page, but there are several versions in the codebase. Should they be made to agree?
+
  * Copyright (C) 2006-2008 Authors
 
+
  * Released under GNU GPL, read the file 'COPYING' for more information
== Documentation ==
+
  */
* Go through .h files and put a sentence or two comment at the top explaining what the class is.  Don't be too detailed; details belong in the comments in the corresponding .cpp file.
+
* Every source file should have the following Emacs local variable block and Vim modeline at the end:
 
+
  /*
* Add comments to each function in .cpp files.  Pick a .cpp file and read through it.  Before each function, add comments describing what the function does. See the files in the inkscape/extensions/ directory as examples. We want ALL of the Inkscape sourcecode documented like that.
+
  Local Variables:
 
+
  mode:c++
== config.h (Done by [[GigaClon]], patch posted 05-09-05) ==
+
  c-file-style:"stroustrup"
 
+
  c-file-offsets:((innamespace . 0)(inline-open . 0)(case-label . +))
Replace instances of
+
  indent-tabs-mode:nil
 
+
  fill-column:99
#include <config.h>
+
  End:
 
+
*/
with
+
// vim: filetype=cpp:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:encoding=utf-8:textwidth=99 :
 
+
* Include style. In-tree includes should use quotes, while system headers should use angle brackets. An exception is 2Geom, which should use angle brackets, though it is local (we are preparing for it to become a standalone library one day). Includes in each group should be sorted alphabetically. The path should be relative to the <tt>src/</tt> directory. If there is a config.h include, it should go at the top. Here is an example:
 
 
 
  #ifdef HAVE_CONFIG_H
 
  #ifdef HAVE_CONFIG_H
 
  # include "config.h"
 
  # include "config.h"
 
  #endif
 
  #endif
 +
 +
#include <cairo.h>
 +
#include <cstdio>
 +
#include <glib.h>
 +
#include <math.h>
 +
 +
#include "display/cairo-utils.h"
 +
#include "document.h"
 +
#include "sp-use.h"
 +
#include "xml/node.h"
 +
* Order of the file. Each file should contain the following, in '''precisely''' that order:
 +
** Doxygen <tt>@file</tt> and copyright comment, as above.
 +
** Include guard (only for header files)
 +
** System includes
 +
** Local includes
 +
** Forward declarations (note: using a forward declaration header is usually better, if a suitable one exists)
 +
** Class declarations
 +
** Function declarations
 +
** Global variable declarations (note: global variables should be avoided)
 +
** End of include guard
 +
** Emacs local variables block (see above)
 +
** Vim modeline (see above)
  
Note the "" instead of <>
+
== Documentation ==
 
 
== Booleans ==
 
 
 
* Convert use of gboolean to bool where feasible
 
* Switch from use of TRUE/FALSE to true/false
 
 
 
(does anybody know a good reason for this? Other than code niceness, can it give problems/bugs to have gboolean and bool?)
 
 
 
 
 
== More details needed ==
 
* Marking C-Style casts ([[JonCruz]])
 
(BPF) See [http://www.ip97.com/gcc/C_002b_002b-Dialect-Options.html#index-Wold_002dstyle_002dcast-155 -Wold-style-cast]
 
 
 
* Fixing C-Style casts ([[JonCruz]])
 
(BPF) There could well be quite a lot of these. In some cases, this is because we are including C variables and concepts in C++ files, but there are many
 
cases which I could classify as a True Bill. Do we in fact want to remove all C-Style casts ...
 
  
* Fix all the gcc 3.4.2 warnings (inkblotter)
+
* Many files begin with a Doxygen comment that doesn't contain the <tt>@file</tt> tag. The result is that the comment doesn't document the file, but the first entity declared in the file, usually the <tt>Inkscape</tt> namespace. Go over those files and put the <tt>@file</tt> tag in the comment. Also close the comment after the description, and put the author information in a regular comment (see above).
(BPF) As reported in [Patch 1223928 ] "Convert call(s) to gtk_widget_set_usize ...", all the warnings cam be easily fixed, save for one in canvas-arena.cpp where we apply a C macro to a C++ struct. It is to be hoped that all new code will be warning-free, but I am not sure whether we should be in a hurry to commit patches to fix old code as the benefit in doing so might be small.
+
* Some documentation is useless, for example "constructor" or "destructor". Such comments mark the entity as documented, when in fact it's not. Remove them.
  
* w32 version start being compiled with [[MinGW]] instead of MSVC
+
== Coding style ==
 +
* Replace C-style casts with the appropriate C++ casts. You can compile with <tt>-Wold-style-casts</tt> to find them easily.
 +
** <tt>static_cast</tt> when the conversion is obvious, for example a floating point to integer type.
 +
** <tt>const_cast</tt> if the only difference between the types are <tt>const</tt> qualifiers.
 +
** <tt>reinterpret_cast</tt> if the conversion does not compile with static_cast, for example pointer to integer.
 +
** <tt>dynamic_cast</tt> for downcasting to derived class type.
  
 
== Elimination of old utest tests ==
 
== Elimination of old utest tests ==

Revision as of 19:05, 4 August 2010

Before embarking on any of those tasks, be sure to contact the developers on the mailing list.

Inline function removal

  • In src/xml/repr.h, there are a bunch of inline functions that are trivial wrappers for SPRepr member functions. Your task, should you choose to accept it: replace all calls to those wrapper functions with direct calls to the wrapped member functions, then remove the unused wrapper functions. (mental) (some patches have been applied from Robert Crosbie)
    • mental says don't touch sp_repr_parent and sp_repr_next (mentioned sp_repr_prev, but it doesn't exist)
    • sp_repr_children (same as above?)
    • sp_repr_unparent (same as above?)
    • sp_repr_ref, sp_repr_unref and sp_repr_set_attr are large conversions needing to be done.
    • sp_repr_document, sp_repr_document_root (do we convert these?)
    • sp_repr_add_child, sp_repr_remove_child and sp_repr_change_order are straight conversions.
  • There is a python script that can help for this kind of replacements. Have a look at it: ReplacementScript

Improving headers for compilation speed

When a header file changes, any file that includes it is recompiled. This becomes a problem when a given header file is included by other header files, as it leads to a vast header include tree, wherein a small change to a seemingly minor header file causes massive rebuilds of much of the codebase.

Fortunately for us, there are recognised techniques to mitigate this! Here's what to do:

  • Find a header that includes several other headers. For example, the header "bar.h" might have a class Bar with a member of type Foo*, thus it includes "foo.h" to get the definition of that type. You can prune this by using a 'forward declaration', whenever the Foo entity is not itself used, only pointers and/or references to it:
//#include "foo.h"  /* <-- kill the header include! */
class Foo;          /* <-- replace with a forward declaration */
class Bar {
    Foo* _foo;
};

There are also forward declaration headers that should be used in preference to handcoded forward declarations:

    • XML namespace - #include "xml/xml-forward.h"
    • Extension namespace - #include "extension/extension-forward.h"
    • SPObject related things - #include "forward.h"

(BPF) The C++ FAQ Sheet explains how to code forward declarations (classes that both need to know about each other); To fully understand why, you should study the Pimpl idiom.

Source formatting

  • Source files should use four spaces as indentation and no tabs. Trailing whitespace should be removed.
  • The comment at the top of each file should have the following format. Note that the Doxygen @file comment does not include the author information, which is in a regular multiline comment. Author emails can be obfuscated, but should be real addresses.
/** @file
 * @brief Logarithmic time traveling salesman solver - public interface
 *//*
 * Authors:
 *   J. God Hacker <ihatepizza@gurus.org>
 *   Ellen Epic <epicwin at email dot com>
 *
 * Copyright (C) 2006-2008 Authors
 * Released under GNU GPL, read the file 'COPYING' for more information
 */
  • Every source file should have the following Emacs local variable block and Vim modeline at the end:
/*
  Local Variables:
  mode:c++
  c-file-style:"stroustrup"
  c-file-offsets:((innamespace . 0)(inline-open . 0)(case-label . +))
  indent-tabs-mode:nil
  fill-column:99
  End:
*/
// vim: filetype=cpp:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:encoding=utf-8:textwidth=99 :
  • Include style. In-tree includes should use quotes, while system headers should use angle brackets. An exception is 2Geom, which should use angle brackets, though it is local (we are preparing for it to become a standalone library one day). Includes in each group should be sorted alphabetically. The path should be relative to the src/ directory. If there is a config.h include, it should go at the top. Here is an example:
#ifdef HAVE_CONFIG_H
# include "config.h"
#endif

#include <cairo.h>
#include <cstdio>
#include <glib.h>
#include <math.h>

#include "display/cairo-utils.h"
#include "document.h"
#include "sp-use.h"
#include "xml/node.h"
  • Order of the file. Each file should contain the following, in precisely that order:
    • Doxygen @file and copyright comment, as above.
    • Include guard (only for header files)
    • System includes
    • Local includes
    • Forward declarations (note: using a forward declaration header is usually better, if a suitable one exists)
    • Class declarations
    • Function declarations
    • Global variable declarations (note: global variables should be avoided)
    • End of include guard
    • Emacs local variables block (see above)
    • Vim modeline (see above)

Documentation

  • Many files begin with a Doxygen comment that doesn't contain the @file tag. The result is that the comment doesn't document the file, but the first entity declared in the file, usually the Inkscape namespace. Go over those files and put the @file tag in the comment. Also close the comment after the description, and put the author information in a regular comment (see above).
  • Some documentation is useless, for example "constructor" or "destructor". Such comments mark the entity as documented, when in fact it's not. Remove them.

Coding style

  • Replace C-style casts with the appropriate C++ casts. You can compile with -Wold-style-casts to find them easily.
    • static_cast when the conversion is obvious, for example a floating point to integer type.
    • const_cast if the only difference between the types are const qualifiers.
    • reinterpret_cast if the conversion does not compile with static_cast, for example pointer to integer.
    • dynamic_cast for downcasting to derived class type.

Elimination of old utest tests

It should be double-checked that the old utest tests (also see TestSuite-blueprint are indeed all obsolete. If there happen to be any left that are not obsolete they should of course be converted to the CxxTest framework (if you don't feel up to it, ask me). Finally, the obsolete files should be removed from the repository and Makefiles, making sure that nothing breaks in the process.