-
Notifications
You must be signed in to change notification settings - Fork 0
Coding Guidelines
When you're writing code for open source projects, the most important rule to follow is this: Try to make your code blend in with the existing code.
When in doubt, use the Google C++ Style Guide. It's a treasure trove of C++ advice. Just double Google's indentation -> 2 spaces become 4 spaces; 4 spaces become 8 spaces.
That being said, there are large chunks of Mixxx that are written in slightly differing styles (mainly variable naming conventions). In order to avoid this in the future, it's best for us to have some coding guidelines for developers to follow.
As you change a part of Mixxx, please update it to match this style guide and reformat it with ClangFormat. That way, eventually all of Mixxx will be written in this style. Do not send us patches that are purely cosmetic with respect to source changes -- this is a waste of time since it does not benefit users directly.
Since 2019-05-09 our code formatting rules are defined by the .clang-format configuration file. It can be found in the project root directory and contains settings for ClangFormat. Make use of it by auto-formatting new or modified code segments as soon as you need to touch it!
ClangFormat version >= 4.0 is required.
On Ubuntu, you can install clang format via.
sudo apt-get install clang-format clang-format-4.0
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-4.0 1000
To automatically adjust the code style in every commit, it is recommended to install the pre-commit framework. It will run a few scripts, including clang-format, to ensure the code style guidelines are met.
The required config file, including installation instructions, is in the Mixxx repository at .pre-commit-config.yaml. Please note that it requires at least Python 3.7. Here are instructions for updating python3 on Ubuntu LTS.
Alternatively you may use git-clang-format from the command line.
You can for example do this after each commit during development:
git-clang-format HEAD^1
git commit -a --amend
or if the PR is already under review:
git-clang-format upstream/master
git commit -a -m"apply git-clang-format changes"
Apply clang-format to individual source files (only permitted for new files):
clang-format -i -style=file [<file> ...]
clang-format will pick up the .clang-format file from the current or any parent directory of the source file(s).
Code formatting is available in Visual Studio Code. Install the C/C++ Extension and verify or update the following settings:
- C_Cpp.formatting = Default (default)
- C_Cpp.clang_format_style = file (default)
- C_Cpp.clang_format_fallbackStyle = Google
- C_Cpp.clang_format_sortIncludes = null (default)
- C_Cpp.clang_format_path = null (default) or path to your clang-format executable (optional)
Don't enable auto-formatting on save, because this will add unnecessary noise to your pull request.
- editor.formatOnSave = false
- editor.formatOnType = true
KDevelop has a clang-tidy plugin.
If you use Eclipse as IDE, the code style "K&R" works well with these tweaks
- tab policy = space only
- new lines = before colon in constructor initializer list
Please note that there are still some exceptions, so do not auto-format a whole file.
In addition you can install https://marketplace.eclipse.org/content/cppstyle to use clang-format from the GUI
Make sure the ClangFormat plugin is installed and enable it in the Settings under Editor > Code Style.
Mixxx's old developers more or less used the convention that indents are 4 spaces. The consensus is that we should try to stick to this, if only for consistency. If your code uses tabs, or does not use 4-space indent then you will be asked to change it.
Please configure your editor to have a max column-width of 80-columns. While it is not a strict requirement, 80-column cleanliness makes it easy to tile multiple buffers of code across a laptop screen, which provides significant efficiency gains to developers.
For Mixxx's clang-format compatibility (ColumnLimit: 0):
- Use double indent (8-spaces) for broken lines (ContinuationIndent: 8).
- Break line after binary operators.
- If you break a list of function parameters, put each parameter on a single line (BinPackArguments: false & BinPackParameters: false).
We add 8 spaces of indentation for broken lines, which helps to easily distinguish them from logical blocks.
Good:
if (long_expression1 ||
long_expression2 ||
long_expression_with_along_parameter_1(parameter1) {
long_expression_with_along_parameter_2(parameter1)
}
Bad:
if (long_expression1 ||
long_expression2 ||
long_expression_with_along_parameter_1(parameter1) {
long_expression_with_along_parameter_2(parameter1)
}
This is an overview of the various conventions that the Mixxx team follows when writing Mixxx code. Most of these conventions are not intended to start holy-wars, rather they are simply intended to keep the Mixxx codebase consistent. You should strive to follow these guidelines as much as possible. If you do not, we may ask you to clean up your code to follow the style guide during code-review.
What's in a name?
Give variables and classes a descriptive but succinct name.
Avoid: Variable names that do not give a hint for their purpose. Examples: ix, i, index, position, name, foo, bar
Local variables should follow either a camelBack case or lowercase_with_underscores style:
QString hotcue_name;
int composerSortOrder;
Pointers should be prefixed with a "p" to indicate they are a pointer. The "*" should be aligned with the type and not the name.
Examples:"
int* pHotcueIndex;
Optionally include minimal type information with variables. This can be handy to know at a glance the rough type of an object. For example, for a ControlObject a common pattern is to prepend "CO" to the variable name:
ControlObject* m_pCOPlayButton;
- Class names must be in CamelCase (e.g. "MyName"), with each word capitalized.
- Member functions of classes must be camelBack cased (e.g. "thisFunction")
- Member variables must be prefixed with "m_". It is essential to know the scope of a variable is at the class-member level with a simple glance for easy readability of source files.
- Class variables (static class members) must be prefixed with "s_".
Braces should not be given their own newline. They should always be separated from surrounding code by a single space.
Good:
if (expression) {
// something
} else {
// something else
}
if (long_expression1 ||
expression2) {
// something
statement
}
Bad:
if (expression)
{
}
else
{
}
Please refer to the Google C++ Style Guide's namespace section.
In new code, please wrap the code in the "mixxx" namespace (avoid making a hierarchy of namespaces). Put module-local helper functions in an anonymous namespace.
#include <SomeQtHeaderUsedInThisHeader>
#include "some/mixxx/file/used/in/this/header.h"
namespace mixxx {
class MyClass {
public:
MyClass();
private:
bool m_helper;
};
bool myFunction();
} // namespace mixxx
#include "path/to/myclass.h"
#include <LocallyNeededQtHeader>
#include "locally/needed/mixxx/header.h"
namespace mixxx {
namespace {
// Put file-local helper functions and constants here.
bool myHelper() {
return true;
}
} // namespace
// Put class implementations, functions, and constants
// that are meant to be used by other parts of Mixxx here.
MyClass::MyClass(): m_helper(myHelper()) {
}
bool myFunction() {
return myHelper();
}
} // namespace mixxx
One-line statements following an if
clause are acceptable, but it is
preferable to wrap them in braces since someone may add a line at the
same indentation level, not realizing that there are no braces.
OK:
if (expression)
statement
There should be one space following the if
keyword, and one space
following the closing parenthesis of the condition before the brace.
Better:
if (expression) {
}
Do not add padding inside the conditional or omit a space between the
if
keyword and the conditional parenthesis.
Bad:
if( expression ){
}
Do not manipulate variables inside conditional expressions. Variable
assignment should be on its own line (except in the opening line of
for
loops).
OK:
++counter;
if (counter > threshold) {
}
Bad:
if (++counter > threshold) {
}
Similarly to if
-statements, there should be a single-space of padding
after the for
keyword and after the closing conditional parenthesis.
Additionally, a single-space padding should come after the semicolon
separators between the initializer, conditional, and increment
statements.
Good:
for (initializer; conditional; increment) {
}
Comments should be complete, descriptive sentences in the present tense. If a comment is a warning or something that might need to be re-evaluated in the future, date the comment with the current month and year, along with your user name.
Good:
// This is required because we don't have enough foo's in the bar -- rryan 2/2011
doSomething();
Prefer to use C++-style comments. All trailing line comments start with a single space after the separator.
Avoid to use C-style comments or Java-style multi-line comments.
Bad:
/*
* Java-style comment
*/
thisCommentIsReallyVerboseFactoryMethodInjectorObserver()
/* C-style comment -- avoid because you can't nest them */
Avoid comments that do not add more information than the words contained in the statement that follows them. Instead, write a descriptive summary of what the following lines accomplish.
Bad:
/* init boofar */
initBoofar();
Avoid to comment out code unless a preceding textual comment explains the exact purpose of the following lines and why this code needs to be preserved as a comment. Commented out debugging statements without an additional description are acceptable if the intend is obvious.
Use ///
for documentation comments above class and member definitions
in header files. These comments are shown in IDEs associated with the
symbol.
Add a few sentences to explain the purpose and use of a class to its header file for every new class you add. Much of the Mixxx code does not have any documentation, so add it whenever you edit a class.
If you'd like to leave a TODO
for yourself, format them like this:
// TODO(rryan) Make sure to double-check this.
If you'd like to leave a general TODO
for the team, use the name
XXX
:
// TODO(XXX) Make this more general
Remember to actually go back and investigate your TODO
's :).
Bad:
// TODO Make this more general
This section outlines our various standards for writing header (.h) files.
If you wish to credit yourself and leave a notice for who people should contact for help about a file, do so in the most brief way possible. Do not insert ASCII art. Do not include a copyright notice, or license because the project has a root LICENSE file which covers these declarations, and so any file-level declarations would be redundant.
// filename.h
// Created 2/21/2011 by RJ Ryan <email>
For new header files use a #pragma once directive instead of #include guards:
#pragma once
There are many header files with legacy include guards like:
#ifndef LIBRARY_H
#define LIBRARY_H
#endif
Replace these with #pragma once
when you edit a header file.
File includes should be done in the following order:
- System Includes
- Qt Includes
- Includes of Mixxx library dependencies
- Mixxx local includes
- Class forward-declarations
Each different group of includes should be separated by a single empty
line. Order the includes in alphabetical order. Relative includes
should never be used. Always include Mixxx local files by specifying
them from the root of the src
folder. Do not forward declare any
classes other than Mixxx project classes.
Example:
#include <math.h>
#include <sys/types.h>
#include <QtCore>
#include <QtDebug>
#include <QTreeView>
#include <taglib/taglib.h>
#include "library/library.h"
class Cue;
The declaration of methods and member variables for classes should be done in the following order:
- Q_OBJECT macro, indented 4 spaces
- Public enums, constants, inner class declarations, etc.
- Public methods
- Public variables (avoid)
- Public
slots
- Qt Signals
- Protected enums, constants, inner class declarations, etc.
- Protected
slots
- Protected methods
- Protected variables
- Private enums, constants, inner class declarations, etc.
- Private
slots
- Private methods
- Private member variables
A couple guidelines for class declarations:
- Each access specifier should be indented 2 spaces.
- Every different section (e.g. private methods, private member variables) should be separated by a single space.
- At your discretion you may insert separating lines between each method if it improves readability.
- Every public method in a class should be accompanied by a comment describing what it does and how it should be invoked. This is the only place that the overall functioning of member methods should be documented. The implementation of a member method should only have comments about the implementation.
- All destructors should have the 'virtual' keyword.
- Consider marking single-argument constructors explicit so that they are not automatically invoked accidentally in assignment.
- Use C++-style comments, not C-style or Java-style comments.
- All class declarations should be preceded with a brief description of the class and its purpose.
Example:
// The Library is the manager class for all library functionality. It contains the LibraryFeature's
// enabled for use with the library and connects them and their signals to the GUI's library widgets.
class Library : public QObject {
Q_OBJECT
public:
Library(QObject* pParent,
ConfigObject<ConfigValue>* pConfig,
bool firstRun);
virtual ~Library();
// bindWidget gives the Library a chance to insert logic into the library
// widgets (WLibrary and WLibrarySidebar).
void bindWidget(
WLibrarySidebar* sidebarWidget,
WLibrary* libraryWidget,
MixxxKeyboard* pKeyboard);
// Add a LibraryFeature to the list of features enabled in the Library.
void addFeature(LibraryFeature* feature);
public slots:
// Request that the Library switch to the default track table and show the
// provided TrackModel.
void slotShowTrackModel(QAbstractItemModel* model);
// Request that the library switch to the provided view name. Views should be
// registered in LibraryFeature::bindWidget() or Library::bindWidget
void slotSwitchToView(const QString& view);
signals:
// Broadcast to the associated WLibrary widget that the provided model
// should be switched to.
void showTrackModel(QAbstractItemModel* model);
// Broadcast to the associated WLibrary widget that the view with the
// provided name should be switched to.
void switchToView(const QString& view);
private:
ConfigObject<ConfigValue>* m_pConfig;
// List of LibraryFeature's enabled in the Library
QList<LibraryFeature*> m_features;
};
Using the goto
statement is not allowed. Typically uses of goto
are
better handled by the C++ idiom of Resource Acquisition Is
Initialization.
- Follow the isocpp no naked new
rule.
Instead of using
new
anddelete
statements, use one of the following "smart pointers" to convey ownership right from the start: -
std::unique_ptr
for exclusive ownership: e.g.auto p = std::make_unique<T>(...)
-
std::shared_ptr
for shared ownership: e.g.auto p = std::make_shared<T>(...)
-
parented_ptr
for ownership by the QT object tree: e.g.auto p = make_parented<T>(...)
- Pass reference
const T&
orT&
if no ownership is transfered. Pass raw pointerconst T*
orT*
if no ownership is transfered andnullptr
is a valid argument value. A function which receives a reference or raw pointer should not hold onto it outside the scope of the function (e.g. by storing it in a member variable). An exception is the parent pointer passed into the constructor of aQObject
and internally stored as a non owning reference.
- Pass smart pointers by value if ownership is transfered
(
std::unique_ptr
) or shared (std::shared_ptr
). Do not pass smart pointers by reference.
- Use
make_parented
to create objects derived fromQObject
that will be assigned to a parent (and will therefore be managed by the Qt object tree). The created object must get a parent before theparented_ptr
is destructed. Example:
auto pBrowseButton = make_parented<QPushButton>(tr("Browse"));
// *pBrowseButton is not assigned to a parent yet
pLayout->addWidget(pBrowseButton);
// Now *pLayout is the parent
- The destructor of
parented_ptr
asserts that the pointed-to object actually has a parent. This ensures that the pointed-to object isn't leaked. A consequence is that aparented_ptr
must never dangle. Never store aparented_ptr
in an object that outlives the parent. This is most easily done by storing theparented_ptr
inside exact the parent. If the lifetime of the pointer relative to the parent is not clear then store the output ofparented_ptr::toWeakRef()
instead of aparented_ptr
.
- Any exceptions to these guidelines must be clearly documented inline in the code.
Use Mixxx assert macros form util/assert.h to assert an assumption that can never fail.
The DEBUG_ASSERT macro works only in debug builds. When the assert
fails, a message is logged. If Mixxx is compiled with scons debug_assertions_fatal=1
it will be terminated after that. If Mixxx is
started under a debugger with mixxx --debugAssertBreak
it will break
(SIGINT) in that case.
Use VERIFY_OR_DEBUG_ASSERT if an error handling is required to recover gracefully in a release build.
Good
#include <util/assert.h>
...
bool failed = neverFails();
VERIFY_OR_DEBUG_ASSERT(!failed) {
qWarning() << "neverFails() has failed"
return;
}
Use QStringLiteral. This has a variety of benefits.
Escape non ASCII characters:
const QString kMessage = QStringLiteral("Hello I\u2019ve to go");
Due to copy/move elision in C++11 no temporary objects will be created by the assignment. Prefer this style for all new code to improve readability.
QChars can be initialized with ASCII characters 16 bit Unicode L’\u00fc' if required. Both are constexpr.
constexpr QChar kc = 'c'
constexpr QChar kue = L'\u00fc' // for "ü"
From Mixxx 2.3 we set QT_USE_QSTRINGBUILDER to use QStringBuilder for operator+. Use + in favour of % for better readability.
In the C++ community are two mutually exclusive styles established:
1.) Google/Qt: "References can be confusing, as they have value syntax but pointer semantics." https://google.github.io/styleguide/cppguide.html#Reference_Arguments
2.) Cpp Core Guidelines: "This makes it clear to callers that the object is assumed to be modified" https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f17-for-in-out-parameters-pass-by-reference-to-non-const
Since we follow the Google style we avoid using value syntax to change variables via a lvalue reference. Using rvalue references to access nested variables is permitted.
Good:
T& X::refIObject();
x.refObject().setValue() // The effected object is still part of the statement
T* pObject = &refObject();
...
pObject->setValue(5); // The pointer syntax reveals the an external object is written
Bad:
T& X::refIObject();
T& object = refObject();
...
object->setValue(5); // It looks like a local variable is changed.
As of the Mixxx 2.3 release, Mixxx is switching to C++17. We are taking a conservative approach to adopting C++11/14/17 features and whitelisting them one by one. If a C++11/14/17 feature you would like to use is not listed here, please email mixxx-devel to make a case for it and we will consider whitelisting it.
We are limited to what is supported across our 3 supported compilers:
In general, Microsoft Visual Studio is the one that prevents us from using features.
See also: C++11 features in Qt5.
http://en.cppreference.com/w/cpp/language/list_initialization
Use with caution!
WARNING on windows, they are not well supported with QT4. see the following links for details
- https://sourceforge.net/p/mixxx/mailman/message/34805913/
- https://bugs.launchpad.net/mixxx/+bug/1627826
- https://github.com/mixxxdj/mixxx/pull/1019
Use!
Yes! Always prefer to NULL.
Yes! Prefer over QScopedPointer.
By including the custom file "util/memory.h" instead of the system header <memory> you are even able to make use of std::make_unique<>() that was missing from the C++11 specification and is now provided by C++14. This generic function should always be used instead of operator new for the allocation of objects that are wrapped in a unique_ptr.
If possible, prefer std::unique_ptr in std:: containers over QSharedPointer in Qt containers.
Do not use unique_ptr for QObjects which have a parent, in this case the parent has the ownership. An early delete due to a smart pointer emit unnecessary signals.
Prefer std::unique_ptr over the Qt Object tree to manage the lifetime of an object.
Side note: Deleting sender objects before receiver objects is faster, because the sender owns the connection.
Use.
Use.
Use.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1757.html
Yes!
Use! Any case where a method intentionally overrides that of a parent class should use the override keyword.
In contrast to the pre-C++11 rules (see above) when using override on a function in a derived class it is recommended to omit the redundant virtual keyword, because override implies virtual.
Destructors in derived classes should also be marked with override instead of virtual. This ensures at compile time that the base class has declared a virtual destructor. If the base class has not declared a virtual destructor the destructor of a derived class might not be invoked. This applies also for default destructors even if it looks noisy.
~ClassName() override = default;
Use (support added in VS2015). Some positive reports from another project.
Appears to be supported by MSVC 2013. Use!
Use them! Prefer them over Qt's foreach
macro which has enjoyed
widespread usage throughout the Mixxx codebase.
Be very careful when updating old code from foreach to range-based-for. It is very easy to write a bug.
for (const QString& item : list) {
qDebug() << "WOOHOO" << item;
}
We do not follow a aaa-style (almost-always-auto).
Our rules of thumb:
- use auto to prevent repeating yourself
- auto when the type is long and does not add any value
Auto is acceptable in function-level scopes (i.e. only within function bodies). Do not use auto in a function or method prototype as this makes the code less self-documenting and would require reading the body of the function to determine its return type or arguments.
More details can be found here: https://google.github.io/styleguide/cppguide.html#auto
Good:
void mixAudio(const QVector<QString>& values, QVector<EngineChannel*>* channels) {
auto bufferSize = 256; // this avoids uninitialised locals
for (const auto& value : values) { // Type can't get wrong
qDebug() << "blah:" << value;
}
for (auto* channel : *channels) {
channel->process(bufferSize);
}
}
Bad:
auto mixAudio(const QVector<QString>& values, QVector<EngineChannel*>* channels) -> bool {
return true; // need to read the function body
}
Bad:
auto value getValue(); // because the type is out of sight
Bad:
QVector<RowInfo>::iterator it = rowInfo.begin();
// Explicit type is only clutter and can be easily guessed from the context.
Bad:
QList<int>* pList = new QList<int>(); // do not repeat yourself
Use carefully! Closures can obscure the control flow of a system and introduce tricky data lifetime issues. Using a lambda in your code will require more time in code review and you may be asked to re-write the code to avoid the usage by your code reviewer. Before using a closure, please consider whether it's truly necessary.
Use 8 space indent for line break or reasonable alignment + 4 indent for a new logical block. Always break after "},"
Good:
m_pTrackCollection->callSync(
[this](TrackCollectionPrivate* pTrackCollectionPrivate) {
connect(&pTrackCollectionPrivate->getCrateDAO(),
SIGNAL(added(int)), this, SLOT(slotCrateAdded(int)));
connect(&pTrackCollectionPrivate->getCrateDAO(),
SIGNAL(renamed(int, QString)), this,
SLOT(slotCrateRenamed(int, QString)));
connect(&pTrackCollectionPrivate->getCrateDAO(),
SIGNAL(deleted(int)), this,
SLOT(slotCrateDeleted(int)));
connect(&pTrackCollectionPrivate->getCrateDAO(),
SIGNAL(autoDjChanged(int, bool)), this,
SLOT(slotCrateAutoDjChanged(int, bool)));
},
__PRETTY_FUNCTION__);
Email mixxx-devel with your use case.
In general, do not use. Do not add gratuitous move constructors -- this decision should be motivated by profiling data to help avoid needless copies.
Note: auto&& is a universal reference and not an r-value reference! It should be used instead of an explicit l-value auto& reference in range-based for loops over non-const (= mutable) containers as shown below.
Good:
for (auto&& item : mutableContainer) {
// read and write item
}
for (const auto& item : immutableContainer) {
// read only item
}
The universal reference auto&& could be used in both cases, but prefer const auto& if you don't intend to modify the item inside the loop's body.
Use. Both the name of the enum type and its enumerated values should be written in CamelCase with the first letter capitalized. Do not add new C style enums and please replace those with enum classes when you are working on code that uses them.
enum class ChannelLayout {
Unknown,
Mono, // 1 channel
DualMono, // 2 channels with identical signals
Stereo, // 2 independent channels left/right
// ...
};
For now, prefer using Qt atomic primitives. We may switch at some point... atomically. 8-)
Use sparingly (supported in VS2015). Consider the readability implications.
Use sparingly. While very handy, these can hamper readability since you now need to look in multiple places (the constructor member initializer list, constructor body, and the member variable declarations) to see what value member state takes on.
Rule of thumb: If a class has non-inline constructors or is more than 100 lines long, prefer initializing everything in the constructor.
Good:
class Helper {
public:
Helper() {}
Helper(int baz): baz(baz) {}
int derivedResult() {
return foo * bar * baz;
}
private:
int foo = 1;
int bar = 2;
int baz = 3;
};
Avoid:
class SomeClass {
public:
SomeClass(int baz); // Defined in some other file.
int derivedResult() {
return foo * bar * baz;
}
private:
int foo = 1;
int bar = 2;
int baz = 3;
};
C++11 added required thread-safe initialization of static variables. Prior to C++11, it was possible for multiple threads to initialize a static concurrently. To prevent this, it was necessary to use a synchronization helper.
Due to Bug #1653368 we build Mixxx with magic statics disabled on Windows. Until this is fixed, we cannot rely on magic statics.
attribute nodiscard
Use it when not checking the return value is a program error like a memory leak. Don't use it everywhere, like in simple getter functions where the compiler will discard the call if the return value is not used.
Mixxx is a free and open-source DJ software.
Manual
Hardware Compatibility
Reporting Bugs
Getting Involved
Contribution Guidelines
Coding Guidelines
Using Git
Developer Guide
Contributing Mappings
Mixxx Controls
MIDI Scripting
Components JS