Janitorial tasks

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.

gboolean to bool

Convert all instances of gboolean to bool in the codebase, and then test. In theory, you could probably do a mass search and replace, compile it, and then play with it heavily for a couple hours, and if nothing breaks, send in the patch.

However, as a poor soul has found out, a simple search & replace will be dangerous. Old-style signal functions in the code, for example, take addresses of gboolean, and changing these spells disaster. Cave canem!

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.

Class Design Consistency

• 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)

class Foo {
   ...
private:
    Foo(const Foo& other); // no copy
    void operator=(const Foo& other); // no assign
};

Add Assertions

• Look for functions that are using pointer variables without checking against NULL first. Add a call to g_assert( ), g_return_if_fail( ), 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 g_assert( ) 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.

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 to it (as stated):

//#include "foo.h"  /* <-- kill the header include! */
class Foo;          /* <-- replace with a forward declaration */
class Bar {
    Foo* _foo;
};

(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.

• Move class documentation from the .h to the .cpp. We have an automated code documentation generator called Doxygen, that generates HTML docs from comments in the .h or .cpp files. Sometimes people put the code docs in the .h, but this means that whenever you update the docs, everything including that .h has to be recompiled, even if there are no _real_ changes to the code. Instead, move all these comments into the corresponding .cpp file. It's okay to have a short paragraph comment at the top of the .h file explaining what it is.

Cleanup: Whitespace

• 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)

(BPF) I have seen files with some lines indented with tabs. Does this mean that it is not acceptable to fix such (incorrect) indentation (in a separate patch) when we work on those files ...

(BPF) This thread Tabs to Spaces seems very apposite, have there been any further ideas...

• Trailing whitespace is also a non-visible but diff-confusing issue, and should also eventually tracked down and removed. (JonCruz)

Cleanup: Syntactical

• 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.

Cleanup: Modelines

• Make sure all files include the emacs Local Variable block and a vim 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?

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.

• 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.

config.h (Done by GigaClon, patch posted 05-09-05)

Replace instances of

#include <config.h>

with

#ifdef HAVE_CONFIG_H
# include "config.h"
#endif

Note the "" instead of <>

More details needed

• Marking C-Style casts (JonCruz)

(BPF) See -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)

(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.

• w32 version start being compiled with MinGW instead of MSVC