Coding Conventions and Developer Guide

Coding Conventions

CMake Conventions

• cmake variable names are always uppercase. Multi-word variables separated with '_'. For example set (ADD_MPI_LIBS "")

Naming Conventions

• The object member names are lowercase, start with 'm_' and each word is split with an underscore (m_class_member)
• The static object member names are lowercase, start with 's_' and each word is split with an underscore (s_static_member)
• The object names are lowercase and each word is split with an underscore (class_name)
• The template arguments have the first letter of each word capitalized (ProperCase), no underscores (TemplateArgument)
• The local variables are lowercase and the words are separated by underscores (same as for object names, well, it's not easy mistake them)
• The typedef names are lowercase, words separated by underscores, and they end with _t (some_def_t), except the case in which a value of that type is never used (apart from default constructing unnamed objects).
• The class methods and all the functions follow the same convention: lowercase, words separated by underscores
• TODO Getter & Setters?
• TODO class names?
• enum types follow the convention of the object names, but are defined inside the enumtype namespace (enumtype::some_id)

Namespaces:

Namespaces uses the following synthax

namespace the_name {
    namespace nested_name {
    } // namespace nested_name
} // namespace the_name

At current time, the library main namespace is gridtools. All library components should be in this namespace except gridtools::enumtype that refers to enums.

Namespace _impl contains the utilities that are not exposed to the user API.

Q: Change _impl in _detail ?

Formatting

• Always use spaces, never tabs.
• Indent with exactly 4 spaces.
• Try to keep line width no longer than 120 characters. Code in an IDE with large screens looks good with longer lines, but code review tools like github does not play well with long lines.

C++

Scoping

• Never use using keyword in library header files.
• Do not use nested classes, except for metafunctions or classes without non-static data members and non-static member functions
• Place a function's variables in the narrowest scope possible, and initialize at the declaration.

//not ok
int a,b,c,d; //only definition, not initialization
b = 10; 
d = 10;
for(a=0; a<b; ++a)
{
    c=3; //define c in the scope where you use it
}

int b=10; 
int d=10;
for(int a=0; a<b; ++a)
{
    int c=3;
}

Inheritance

• Make sure inheritance models a "is a" relationship, otherwise use composition.

//not ok 
class Bus : public Ship 
...
//ok
class Bus: public Vehicle
...

• Never redefine an inherited non virtual method

class Base
{
    void foo() { printf("base\n");}
}
class Subclass : public Base
{
    void foo() { printf("sub\n"}; //redefinition of foo
}
Subclass* sub = new Subclass();
sub->foo(); //prints sub
static_cast<Base*>(sub)->foo(); //prints base

Includes

• All internal includes in the library are with "".
• <> are used for user code, like in the examples.
• The GridTools is a set of modules, each providing different functionality. The user might want to use only functionality of a module. For each module, i.e. storage, there is a unique header (with the same name as the header) that the user needs to include. For example:
  • storage -> storage/storage.hpp
  • stencil-composition/stencil-composition.hpp
• Paths are always relative, e.g.

#include "../arg.hpp"
#include "structured_grids/esf.hpp"

Classes

• When implementing a class the following structure should be used

  • First declare typedefs/using statements
  • Then declare static data members
  • Followed by data members
  • Then constructors
  • And, at the end the rest of member functions

• You must define a default constructor if your class defines member variables and has no other constructors. Otherwise the compiler will do it for you, badly. For example

class Rectangle
{
public:
    void Rectangle()
    {
        // initialize variables with impossible default value
        // -> debugging easier
        m_width = -1;
        m_heigth = -1;
    }
private:
    int m_width;
    int m_height;
};

• Do never rely on default initialization of compiler

• Always disable the copy constructor and assignment operators of a class (unless you explicitly provide those) by means of the DISALLOW_COPY_AND_ASSIGN macro.

class Foo 
{
    DISALLOW_COPY_AND_ASSIGN(Foo);
public: 
    Foo(int f);
    ~Foo();

• If a class has virtual methods, the destructor must be virtual
• Pass primitive types (int, double, etc.) by value, unless it is an output parameter of the function.
• Other input only arguments are passed by const reference, except the argument can be NULL, then a const pointer is used.

Move semantics

Using && and std::forward is only for when the semantics requires it, not for performance reasons. This depends on the width of the application of the functions accepting the values in question. If a function is never called with a non POD there is no need to specify rvalue-references (unless the state of the object include a validity flag that need to reflect that a move operation happened). If a function accepts objects that allocate on the heap, then the move operations can really be beneficial.

Error checking and report: Exceptions, Static Asserts and Asserts

In general, exceptions are used to inform the user/caller that a function cannot be completed for some reason. Assertions are used to check if an invariant, a precondition, or a postcondition is satisfied. Also, exceptions are error conditions that the user may fix. An assert cannot be fixed.

An exception is thrown when an allocation fails, since it signifies the operation cannot be completed, but the user may be able to fix it. An assert is triggered when the internal state of an object is invalid, and from that moment the computation cannot be trusted, the user does no longer control what's going on.

Another difference between asserts and exceptions is that asserts are not active in release mode. So, the errors that are caught with assertions must be ones that can be fixed in the development phase, but they should not occur in production phase, since this would lead to pretty nasty situations.

So, for checking that the data fields are big enough to contain the iteration space an exception is perfect: maybe the user can adjust the sizes somehow. To check that a stencil composition is valid static_asserts is the way to go, but for checking that the pointers in iterate_domain are not null, asserts should be used. Also because, an exceptions would be really of not use for the user who does not know what the internal state of that class should be.

Variadic Pack overloads

• Always constraint the type of the variadic packs (using the is_pack_of SFINAE) passed to an overloaded method:

   // NOT OK
   template < typename... IntTypes>
   constexpr myclass(IntTypes... dims) {}

   // OK
   template < typename... IntTypes, typename Dummy = is_pack_of< is_static_integral, IntTypes... > >
   constexpr myclass(IntTypes... dims) {}

Pointers

• Use references whenever possible (instead of pointers), i.e. when assignment happens at initialization time and the reference does not change over time.
• Always assert pointer before using them

assert(ptr);
ptr->xx();

• Try to use RAII, i.e. use smart pointers for non performance critical sections:

boost::shared_ptr<MyClass> ptr = boost::make_shared<MyClass>();

• Use delete[] for freeing array of pointers.

metaprograms

• Every type passed as a template argument is checked (asserted). This increase both safety and readability (from trait that checks the type we infer its function)

template<
    typename MssArray, 
    typename Coords, 
    typename MssLocalDomainArray, 
    enumtype::backend BackendId, 
    enumtype::strategy StrategyId
>
struct mss_functor
{
    BOOST_STATIC_ASSERT((
        is_sequence_of<
            MssLocalDomainArray, 
            is_mss_local_domain
        >::value));
    BOOST_STATIC_ASSERT((
        is_meta_array_of<MssArray, is_mss_descriptor>::value
    ));
    BOOST_STATIC_ASSERT((is_coordinates<Coords>::value));
    ....
}

• Avoid "Blob" anti-pattern (see Chapter 2.2 of Book C++ Template Metaprogramming from David Abrahams), where one metafunction computes multiple output. It decreases readability. For example in the following example, run_functor_traits is a blob anti-pattern

template<typename run_functor_derived>
class run_functor
{
   typedef typename run_functor_traits<run_functor_derived>::
       execute_policy execute_policy_t;
   typedef typename run_functor_traits<run_functor_derived>::
       arguments arguments_t;
   typedef typename run_functor_traits<run_functor_derived>::
       local_domain local_domain_t;
}

Replace it by single output type patterns:

template<typename run_functor_derived>
class run_functor
{
   typedef typename run_functor_arguments<run_functor_derived>::type 
      arguments_t;
   typedef typename run_functor_local_domain<run_functor_derived>::type 
      local_domain_t;
}

Developer's Guide

Code Structure

• Different classes with different functionality go into different header files. Do not build .h files with a blob of classes.
• Name of header files is after name of the class. For example local_domain.h contains the local_domain class
• Put metafunctions associated to, or act on a type in a <type>_metafunctions.h file. For example the metafunction

template<...> struct make_interval{...};

will be placed in interval_metafunctions.h

• Put implementation details in <type>_impl (or _detail if the internal namespace is turned into _detail)

Tests

• Everything is tested: runtime functions, metdhos, functors but as well metafunctions. If you write new classes or method, provide the corresponding metafunctions before creating a pull request.
• Write all tests of a class or functionality from <type>.h into a file in the unit_tests folder, named test_<type>.h

Documentation

We document code using Doxygen. The documentation is organized by groups. In Doxygen, groups can be arbitrarily nested, so a group can be defined inside another one. A top-level group will result in the final documentation as a Module, while the nested groups as sub-modules (the itemized list will be indented consequently). Let's look at an example. The file selected for this example is stencil-composition/expressions/expressions.hpp.

namespace gridtools {
    /** \ingroup stencil-composition
        @{
    */

    /** \defgroup expressions Expressions
        @{
    */

    /** Namespace containing all the compomnents to enable using expressions in stencil operators
     */
    namespace expressions {
    } // namespace expressions

    /** @} */
    /** @} */
} // namespace gridtools

This file will define a sub-group of the stencil-composition group, so after having said that the content is part of the stencil composition group, we can define (we can use \ingroup if the group is defined somewhere else) the group for expressions and declare that the whole thing belong there. Notice the double @} at the end since we need to say that we need to close the documentation for both groups.

Remember to put enclose the documentation of your classes in the right group. Usually this is done by using the \ingroup command followed by @{ at the beginning of the file after the gridtools namespace declaration, and by using the @} command at the end before closing the gristools namespace declaration.

For the common folder the situation is slightly different, since it contains different utilities needed by the rest of the code. There we will have a Common group, in which each utility is put in a separate \defgroup.

``

Remember to use \param for regular parameter, \tparam for template parameters, and \return for the returned value if not void, to document the arguments of a function. When the argument name is unnamed, it may be tricky to get Doxygen to not complain about it. I found out that sometime a * in place of the argument name works, but other times a generic x works.

Below is an example

    /**
     * @brief functor that executes all the functors contained within the mss
     * @tparam MssArray meta array containing all the mss descriptors
     * @tparam Coords coordinates
     * @tparam MssLocalDomainArray sequence of mss local domain (each contains the local domain list of a mss)
     * @tparam BackendId id of backend
     * @tparam StrategyId id of strategy
     */
    template<typename MssArray, typename Coords, typename MssLocalDomainArray, enumtype::backend BackendId, enumtype::strategy StrategyId>
    struct mss_functor
    {

        /**
         * @brief This is a do something method
         * @param local_domain_lists this is a param
         * @param coords this is a param
         * @param block_idx this is a param
         * @param block_idy this is a param
         * @return this is the return value
         */
        static int do_smething(MssLocalDomainArray& local_domain_lists, const Coords& coords, const int block_idx, const int block_idy) :
             {...}

We are not enforcing the use of \ or @ for the commands. Some commands requires @, like @{ and @}, but others can use both. While the \ because is less dense, code readability is not impacted by this choice and could be left to the personal taste of the developer.

To make the Doxygen documentation you need doxygen and dot available and you need to run make doc from the build folder.

Code comments

For other comments that are not meant to be parsed by Doxygen, it is important to document non-obvious statements and algorithms. For instance, when calling a meta-function fo iterate over a sequence of types, it would be useful to document why that iteration happen, since the documentation of the meta-function is usually too generic to be applied to the particular use. The programmer should then describe the objective of the function (this should be done at the Doxygen level), plus any intermediate step that may help a future reader to understand the code.

Git & Code Review

• Create one branch per functionality. Avoid personal branches that mix multiple functionality and keep growing.
• Always merge and close a branch after a pull request is completed (do not continue developing in the branch)
• Do not create branches from branches (unless you a have a good reason), always branch from master

Before merging into master

• Any branch candidate for merge into the trunk, for which we have created a pull request should have been compiled and tested for all the different cmake options (at least the ones that generate different compilations), like GPU=ON, OFF, CXX11=ON|OFF, DEBUG|Release, etc.
• master should always be in a healthy state. It must not contain bug nor performance bugs. That means it compiles, and run for all backends and cmake options, and performance numbers for all backends are matching the reference values.
• Check the performance for all backends for the representative benchmarks. Verify that performance was not degraded with respect to reference.