User Tools

Site Tools


coding_guidelines

Differences

This shows you the differences between two versions of the page.

Link to this comparison view

Both sides previous revision Previous revision
Next revision
Previous revision
coding_guidelines [2019/01/20 11:39]
beenisss Typos
coding_guidelines [2019/06/19 07:11] (current)
haslersn [Pointer, Object lifetime/Ownership]
Line 9: Line 9:
 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. ​ 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. 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.** ​+**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.** ​
  
-===== Tabs vs. Spaces ​=====+===== Code Formatting ​=====
  
-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 consistencyIf your code uses tabs, or does not use 4-space indent then you will be asked to change ​it.+Since 2019-05-09 our code formatting rules are defined by the [[https://​github.com/​mixxxdj/​mixxx/​blob/​master/​.clang-format|.clang-format]] configuration file. It can be found in the project root directory and contains settings ​for [[https://​clang.llvm.org/​docs/​ClangFormat.html|ClangFormat]]. **Make ​use of it by auto-formatting new or modified code segments as soon as you need to touch it!**
  
-===== Line Widths =====+ClangFormat version >4.0 is required.
  
-Please configure your editor to have a max column-width of 80-columns. While it is not a strict requirement80-column cleanliness makes it easy to tile multiple buffers of code across a laptop screen, which provides significant efficiency gains to developers. Use double indent (8-spaces) for broken lines or align with the opening "​("​ in the line above for hanging indents.+On Ubuntuyou can install clang format via.
  
-===== Auto-Formatter =====+<code bash> 
 +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 
 +</​code>​
  
-We currently ​do not provide an auto-formatter ​or code-checker+ 
 + 
 + 
 + 
 +==== Command line ==== 
 + 
 +Use [[https://​raw.githubusercontent.com/​llvm-mirror/​clang/​master/​tools/​clang-format/​git-clang-format|git-clang-format]] to make sure your changes follow the mixxx style. 
 + 
 +[[https://​dx13.co.uk/​articles/​2015/​4/​3/​Setting-up-git-clang-format.html|Setting-up git-clang-format]] 
 + 
 +You can for example ​do this after each commit during development:​  
 +<code bash> 
 +git-clang-format HEAD^1 
 +git commit -a --amend 
 +</​code>​ 
 + 
 +or if the PR is already under review:  
 +<code bash> 
 +git-clang-format upstream/​master  
 +git commit -a -m"​apply git-clang-format changes"​ 
 +</​code>​ 
 +  
 + 
 +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). 
 + 
 +==== VisualStudio Code ==== 
 + 
 +Code formatting is available in [[https://​code.visualstudio.com|Visual Studio Code]]. Install the [[https://​marketplace.visualstudio.com/​items?​itemName=ms-vscode.cpptools|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 ==== 
 +[[KDevelop]] has a [[https://​github.com/​KDE/​kdev-clang-tidy|clang-tidy plugin]]. 
 + 
 +==== Eclipse ====
  
 If you use Eclipse as IDE, the code style "​K&​R"​ works well with these tweaks ​ If you use Eclipse as IDE, the code style "​K&​R"​ works well with these tweaks ​
Line 27: Line 77:
   * new lines = before colon in constructor initializer list   * new lines = before colon in constructor initializer list
 Please note that there are still some exceptions, so do not auto-format a whole file. 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 
 +
 +===== Tabs vs. Spaces =====
 +
 +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.
 +
 +===== Line Widths =====
 +
 +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. Use double indent (8-spaces) for broken lines or align with the opening "​("​ in the line above for hanging indents.
 +
 +
  
 ====== C++ Style Guide ====== ====== C++ Style Guide ======
Line 123: Line 185:
 bool myFunction();​ bool myFunction();​
  
- // namespace mixxx +} // namespace mixxx
-// ^ Note two spaces after the closing brace.+
 </​code>​ </​code>​
  
Line 140: Line 201:
 // Put file-local helper functions and constants here. // Put file-local helper functions and constants here.
 bool myHelper() { bool myHelper() {
-  ​return true;+    ​return true;
 } }
  
- // namespace ​ +} // namespace ​
-// ^ Note two spaces after the closing brace.+
  
 // Put class implementations,​ functions, and constants ​ // Put class implementations,​ functions, and constants ​
 // that are meant to be used by other parts of Mixxx here. // that are meant to be used by other parts of Mixxx here.
-MyClass::​MyClass() : m_helper(myHelper()) {+MyClass::​MyClass():​ m_helper(myHelper()) {
 } }
  
 bool myFunction() { bool myFunction() {
-  ​return myHelper();+    ​return myHelper();
 } }
  
- // namespace mixxx +} // namespace mixxx
-// ^ Note two spaces after the closing brace.+
 </​code>​ </​code>​
  
Line 219: Line 278:
 ===== Comments ===== ===== Comments =====
  
-Use C++-style comments only. Do not use C-style comments or Java-syle comments. ​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 username+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:** **Good:**
Line 227: Line 286:
 </​code>​ </​code>​
  
-Plain-text comments ​should be separated from the comment symbol by a single space. ​Commented-out code should have no space between the comment symbol and the code:+Prefer to use C++-style comments. All trailing line comments start with a single space after the separator.
  
-**Good:** +Avoid to use C-style comments or Java-style multi-line comments.
-<code cpp-qt> +
-// Textual comment +
-//if (thisSectionIsDeprecated) { +
-//    // Do something crufty +
-//} +
-</​code>​+
  
 **Bad:** **Bad:**
 <code cpp-qt> <code cpp-qt>
-//​Textual ​comment +/
-// if (thisSectionIsDeprecated+ * Java-style ​comment 
-//     // Do something crufty + * 
-// }+thisCommentIsReallyVerboseFactoryMethodInjectorObserver() 
 +/* C-style comment -- avoid because you can't nest them */
 </​code>​ </​code>​
- 
  
 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. 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.
Line 254: Line 307:
 </​code>​ </​code>​
  
-**Bad:** +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.
-<code cpp-qt>​ +
-/* +
- * Java-style ​comment +
- */  +
-thisCommentIsReallyVerboseFactoryMethodInjectorObserver() +
-/* C-style comment -- avoid because you can't nest them */ +
-</code+
 ==== TODO's ==== ==== TODO's ====
  
Line 379: Line 424:
     // bindWidget gives the Library a chance to insert logic into the library ​     // bindWidget gives the Library a chance to insert logic into the library ​
     // widgets (WLibrary and WLibrarySidebar).     // widgets (WLibrary and WLibrarySidebar).
-    void bindWidget(WLibrarySidebar* sidebarWidget,​ +    void bindWidget( 
-                    WLibrary* libraryWidget,​ +            ​WLibrarySidebar* sidebarWidget,​ 
-                    MixxxKeyboard* pKeyboard);+            WLibrary* libraryWidget,​ 
 +            MixxxKeyboard* pKeyboard);
                     ​                     ​
     // Add a LibraryFeature to the list of features enabled in the Library.     // Add a LibraryFeature to the list of features enabled in the Library.
Line 419: Line 465:
  
  
-===== Pointer, Object ​live time / Ownership ​ =====+===== Pointer, Object ​lifetime/​Ownership ​ =====
  
   * Follow the isocpp [[https://​github.com/​isocpp/​CppCoreGuidelines/​blob/​master/​CppCoreGuidelines.md#​es60-avoid-new-and-delete-outside-resource-management-functions | no naked new rule]]. Instead of using ''​new''​ and ''​delete''​ statements, use one of the following "smart pointers"​ to convey ownership right from the start:   * Follow the isocpp [[https://​github.com/​isocpp/​CppCoreGuidelines/​blob/​master/​CppCoreGuidelines.md#​es60-avoid-new-and-delete-outside-resource-management-functions | no naked new rule]]. Instead of using ''​new''​ and ''​delete''​ statements, use one of the following "smart pointers"​ to convey ownership right from the start:
Line 426: Line 472:
     * ''​parented_ptr''​ for ownership by the QT object tree: e.g. ''​auto p = make_parented<​T>​(...)''​     * ''​parented_ptr''​ for ownership by the QT object tree: e.g. ''​auto p = make_parented<​T>​(...)''​
  
-  * Pass raw pointer ''​T*''​ if no ownership is transfered. A function which receives a raw pointer should not hold onto the pointer ​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 a ''​QObject''​ and internally stored as a non owning reference. Do not pass smart pointers by reference. ​+  * Pass reference ''​const T&''​ or ''​T&''​ if no ownership is transfered. ​Pass raw pointer ​''​const T*''​ or ''​T*''​ if no ownership is transfered ​and ''​nullptr''​ 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 a ''​QObject''​ 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.
  
-  * ''​parented_ptr'' ​requires the parent ​to be passed into the constructor at the time of creation. If pointer that will eventually ​be managed by the QT object tree needs to be temporarily ​created ​without ​a parent, first create ​the object ''​p''​ with ''​std::​make_unique'',​ and then later create a new variable using ''​parented_ptr(p.release())'' ​once the underlying object gets a parent.+  * Use ''​make_parented''​ to create objects derived from ''​QObject''​ that will be assigned to parent (and will therefore ​be managed by the Qt object tree). The created ​object must get a parent ​before ​the ''​parented_ptr'' ​is destructedExample:
  
-  ​Never store a ''​parented_ptr''​ in an object that outlives the parent. If the lifetime of the child relative to the parent ​is not clear then store the output of ''​parented_ptr::​toWeakRef()''​ instead.+    auto pBrowseButton = make_parented<​QPushButton>​(tr("​Browse"​));​ 
 +    // *pBrowseButton ​is not assigned to a parent yet 
 +    pLayout->​addWidget(pBrowseButton)
 +    // Now *pLayout is the parent
  
-  * Any exceptions ​to these guidelines ​must be clearly documented inline ​in the code+  * 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 **a ''​parented_ptr'' ​must never dangle**. Never store a ''​parented_ptr'' ​in an object that outlives the parent. This is most easily done by storing the ''​parented_ptr''​ inside exact the parent. If the lifetime of the pointer relative to the parent is not clear then store the output of ''​parented_ptr::​toWeakRef()''​ instead of a ''​parented_ptr''​.
  
 +  * Any exceptions to these guidelines must be clearly documented inline in the code.
  
 ===== Assertions ===== ===== Assertions =====
Line 470: Line 522:
  
  
-===== C++11 =====+===== C++14 =====
  
-As of the Mixxx 2.release, Mixxx is switching to C++11. We are taking a conservative approach to adopting C++11 features and whitelisting them one by one. If a C++11 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.+As of the Mixxx 2.release, Mixxx is switching to C++14. We are taking a conservative approach to adopting C++11/14 features and whitelisting them one by one. If a C++11/14 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: We are limited to what is supported across our 3 supported compilers:
-  * [[http://​clang.llvm.org/​cxx_status.html|Clang 3.3]] +  * [[http://​clang.llvm.org/​cxx_status.html|Clang ​>= 3.4]] 
-  * [[https://​gcc.gnu.org/​projects/​cxx0x.html|GCC ​4.8]] +  * [[https://​gcc.gnu.org/​projects/​cxx0x.html|GCC ​>= 5.4 (Ubuntu Xenial)]] 
-  * [[https://​msdn.microsoft.com/​en-us/​library/​hh567368.aspx|Microsoft Visual Studio ​2015 Update 3]]+  * [[https://​msdn.microsoft.com/​en-us/​library/​hh567368.aspx|Microsoft Visual Studio ​2017 (_MSC_VER >= 1910)]]
  
 In general, Microsoft Visual Studio is the one that prevents us from using features. In general, Microsoft Visual Studio is the one that prevents us from using features.
Line 585: Line 637:
   auto bufferSize = 256; // this avoids uninitialised locals   auto bufferSize = 256; // this avoids uninitialised locals
   ​   ​
-  for (const auto& value: values) { // Type can't get wrong +  for (const auto& value : values) { // Type can't get wrong 
       qDebug() << "​blah:"​ << value;       qDebug() << "​blah:"​ << value;
   }   }
   ​   ​
-  for (auto* channel: *channels) {+  for (auto* channel : *channels) {
       channel->​process(bufferSize);​       channel->​process(bufferSize);​
   }   }
Line 660: Line 712:
 **Good:** **Good:**
 <code cpp-qt> <code cpp-qt>
-for (auto&&​ item: mutableContainer) {+for (auto&&​ item : mutableContainer) {
   // read and write item   // read and write item
  
  
-for (const auto& item: immutableContainer) {+for (const auto& item : immutableContainer) {
   // read only item   // read only item
  
Line 673: Line 725:
 ==== forward declared / strongly typed enums ==== ==== forward declared / strongly typed enums ====
  
-Use. Both the name of the enum type and its enumerated values should be written in CamelCase with the first letter capitalized.+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.
  
 <code cpp-qt> <code cpp-qt>
 enum class ChannelLayout { enum class ChannelLayout {
     Unknown,     Unknown,
-    Mono,     ​// 1 channel+    Mono, // 1 channel
     DualMono, // 2 channels with identical signals     DualMono, // 2 channels with identical signals
-    Stereo, ​  ​// 2 independent channels left/right+    Stereo, // 2 independent channels left/right
     // ...     // ...
 }; };
Line 703: Line 755:
   public:   public:
     Helper() {}     Helper() {}
-    Helper(int baz) : baz(baz) {}+    Helper(int baz): baz(baz) {}
     ​     ​
     int derivedResult() {     int derivedResult() {
-      ​return foo * bar * baz;+        ​return foo * bar * baz;
     }     }
     ​     ​
Line 723: Line 775:
     ​     ​
     int derivedResult() {     int derivedResult() {
-      ​return foo * bar * baz;+        ​return foo * bar * baz;
     }     }
     ​     ​
coding_guidelines.1548002385.txt.gz · Last modified: 2019/01/20 11:39 by beenisss