Contribution guide

This is a collection of styles, best practices and contribution manuals I’m using in a various projects.

Git

Preparing pull request

Never use tabs

Don’t use tabs in code. How they are interpreted depends on the IDE/environment so everybody will see different picture. Mixed with the spaces makes code completely unreadable.

Here is a list of coding style guides of few projects:

  • Qt Spaces, not tabs!
  • Google Do not use tabs in your code
  • LLVM In all cases, prefer spaces to tabs in source files
  • Boost use spaces instead of tabs
  • Mozilla No tabs
  • KDE No tabs

Stackoverflow

According to Developer Survey Results 2017:

Well, 42.9% of developers use tabs. And 37.8% think that group is wrong.
(38,851 responses)

Heh :)

Inserting spaces on pressing TAB
Highlight tabs in text editor
Removing tabs from file
  • Use sed -i 's,\t,  ,g' main.cpp on Linux
  • Use gsed -i 's,\t,  ,g' main.cpp on OSX
  • In gVim you can execute :%s,\t,  ,g command

See also

Google style guide - Spaces vs Tabs

Remove trailing whitespaces

Trailing whitespaces doesn’t change code execution flow, may be not visible to user but add additional mess with the diffs.

Note

In Xcode trailing whitespace will not be trimmed on file save if it’s in the same line with the cursor.

See also

Google style guide - Horizontal Whitespace

Check there is a new line at end of file

You can see this warning while creating GitHub pull request:

_images/github-diff-no-newline-warning.png

Please fix this warning by adding new line at end of file.

Stackoverflow

Set your copyrights

Usually there is a copyright note at the top of the source file. Please add your name there. Update year if necessary.

For C++ code:

// Copyright (c) 2015-2016, Ruslan Baratov
// All rights reserved.

For CMake code:

# Copyright (c) 2015-2016, Ruslan Baratov
# All rights reserved.

For RST documentation:

.. Copyright (c) 2015-2016, Ruslan Baratov
.. All rights reserved.

Keep git commit’s summary short

Long summary makes output of git branch -vv looks bad:

_images/git-branch-verbose-output-of-long-summary.png

Same with the git log --pretty=oneline output.

On GitHub summary can be used as a topic of the pull request. So it will looks ugly too:

_images/github-pull-request-long-summary.png

See also

Linux recommendation

Margin 80 characters

Keep each line of the code so line length is 80 characters maximum.

  • For gVim you can highlight margin using join command, see vimrc example
  • This rule can be ignored for hyper-links

See also

Google style guide - Line Length

Keep each commit as trivial as possible

Keep your commits as trivial as possible, do not mix non-related changes in one commit. For example:

  • Do not mix renaming of variable with changes in logic - make two commits:
  1. rename variable
  2. apply other changes
  • Do not rename file and do change of it’s internals - make two commits:
  1. rename file
  2. change internals
  • Quite the same with big blocks of code. If block moves with a small change make two commits:
  1. move big block only
  2. change code in block

In short keep in mind that cosmetic/trivial changes is quite easy to review (unlike logic) but they can add additional mess.

Note

Qt Wiki (make atomic commits...)

Submitting pull request

Pick target branch

Usually default branch is called master. If there is one more branch called develop send a patch as a pull request against it. After successful testing this branch will be merged to master.

Read CONTRIBUTING

If there is CONTRIBUTING file in repository you will see guidelines for contributing link. Please read it before submit :)

_images/github-guidelines-for-contributing.png

See also

Updating pull request

Notify

Please note that when you’re updating pull request (by doing git push origin <branch-linked-to-pr>) no notification will be send to anybody notification will be send but it’s not clear is the pull request ready or it’s just a part of work-in-progress update. So to avoid confusion when you’re done leave the “ping” note, like Updated!.

See also

C++

... WORK-IN-PROGRESS ...

File extensions

  • .fpp - Forward declaration C Plus Plus
  • .hpp - Header C Plus Plus
  • .ipp - Template Implementation C Plus Plus
  • .cpp - C Plus Plus source file
  • .tpp - Template instantiation C Plus Plus

Note

Example of vimrc

Compilers limitations

Other

CMake

Formatting

No space between command and (

# 'if' is not a keyword, it's a command, like 'execute_process'
if(var1 EQUAL var2)
  command(...)
  command(...)
endif()

Enclose path with double quotes

sugar_foo(sources "./sources")
    # sources - local variable
    # "./sources" - some directory

Next code produce error set_target_properties called with incorrect number of arguments:

set(x "")
set_target_properties(foo PROPERTIES INCLUDE_DIRECTORIES ${x})

Works fine:

set(x "")
set_target_properties(foo PROPERTIES INCLUDE_DIRECTORIES "${x}")

Example with file(WRITE ...):

file(WRITE "${A}/path/to/file/with spaces/in" "message") # quotes required
file(WRITE "${A}/path/to/file/without-spaces/in" "message") # quotes not necessary but keep style the same

Note that quotes required for anything related to configure_file:

# script.cmake.in
file(WRITE ${filename} ${content})

set(filename "/path/with/spaces/A B C")
set(content "a b c")
configure_file(.../script.cmake.in .../script.cmake)
execute_process(COMMAND ${CMAKE_COMMAND} -P .../script.cmake ...)

Result will be creation of file /path/with/spaces/A with content BCabc. Fix:

# script.cmake.in
file(WRITE "${filename}" "${content}")

or

# script.cmake.in
file(WRITE "@filename@" "@content@")

Quite the same happens in install(CODE command:

install(CODE "execute_process(COMMAND ${CMAKE_COMMAND} -E echo hello ...)")

content of cmake_install.cmake will be:

execute_process(COMMAND /.../bin/cmake.exe -E echo hello ...) # no quotes!

it means that if CMake is installed to path with spaces this command will not be executed.

Indentation

2 spaces is default indentation:

command(...)
command(...)
if(...)
  # +2 spaces
  command(...)
  if(...)
    # +2 spaces
  endif()
endif()

function(...)
  # +2 spaces
  command(...)
endfunction()

Use 4 spaces for breaking long line:

command(short line)

command(
    # +4 spaces
    long line arg1 arg2
)

command(
    # +4 spaces
    very
    long
    line
    arg1
    arg2
    arg3
)

alternatively same line can be kept for name-value:

command(
    # +4 spaces
    VALUE1 value1
    VALUE2 value2
    VALUE3 value3
    # break long line with additional indentation
    VALUE4
        # +4 spaces
        value4a
        value4b
        value4c
)

Naming

Lower case for commands:

if(A)
  command(...)
endif()

Upper case for command specifiers:

list(APPEND list_var append_var)

Lower case for local variables (temps, parameters, ...):

foreach(x ${ARGV})
  message(${x})
endforeach()

Upper case for global variables (like variables which Find-modules use/setup):

include(ModuleA) # define MODULE_A_MODE
if(MODULE_A_MODE)
  command(...)
endif()

Lower case for function/macro names:

macro(do_foo)
  command(...)
endmacro()

do_foo(...)

Start internal variable’s name with the _ in macro and headers:

macro(do_foo)
  # command `macro` doesn't introducing new scope
  # hence this `set` commands will pollute user's space
  set(_value1 "...")
  set(_value2 "...")
  # ...
endmacro()

# MyModule.cmake
# same for the header
string(COMPARE EQUAL "${A}" "${B}" _is_equal)
if(_is_equal)
  # ...
endif()

# variable _is_equal not defined
include(MyModule)
# variable _is_equal defined

Examples:

Note

Use functions when it’s possible!

To prevent collisions guard variable name should match path to the module:

# module flags/gcc.cmake from project Polly
if(DEFINED POLLY_FLAGS_GCC_CMAKE_)
  return()
else()
  set(POLLY_FLAGS_GCC_CMAKE_ 1)
endif()

# module cmake/Hunter from project Hunter
if(DEFINED HUNTER_CMAKE_HUNTER_)
  return()
else()
  set(HUNTER_CMAKE_HUNTER_ 1)
endif()

Note

Inspired by Google C++ Style Guide - The #define Guard

Pitfalls

STREQUAL

Usage of if(${A} STREQUAL ${B}) is not recommended, see this SO question. Preferable function is string:

string(COMPARE EQUAL "${A}" "${B}" result)
if(result)
  message("...")
endif()

Note

Fixed in CMake 3.1 by CMP0054 policy

export(PACKAGE ...)

Library of CMake extra modules

  • All defined functions/macros start with <libname>_ (example)
  • no message command inside, only wrappers
  • <libname>_STATUS_PRINT option control message output (default value is ON)
  • <libname>_STATUS_DEBUG option used for more verbose output and additional debug checks (default value is OFF)
  • one function/macro - one file
  • <include-name> equal <function-name>

As the result: include only what you need, check that included function is used by simple in-file search (and, of course, delete it if it’s not). If you need to use sugar_foo_boo function, just include sugar_foo_boo.cmake:

include(sugar_foo_boo) # load sugar_foo_boo.cmake file with sugar_foo_boo function
sugar_foo_boo(some args) # use it

Note about wrappers

Probably some wrappers (like sugar_fatal_error) occupy more space than functionality it is wrapping (: The purpose of this functions is to make additional check. See the difference between this two misprints:

message(FATA_ERROR "SOS!") # Output will be: "FATA_ERRORSOS!", no error report...

include(sugar_fata_error) # include error will be reported

sugar_fata_error(...) # function not found error will be reported

iOS detection

Polly toolchain set IOS variable:

if(IOS)
  # iOS code
endif()

also CMAKE_OSX_SYSROOT can be checked:

string(COMPARE EQUAL "${CMAKE_OSX_SYSROOT}" "iphoneos" is_ios)

Temporary directories

  • ${PROJECT_BINARY_DIR}/_3rdParty/<libname>

Python

... WORK-IN-PROGRESS ...