Difference between revisions of "Coding Standards"
(→Design guidelines: Add LOG) |
Ulmus-scott (talk | contribs) (→Class and variable names: change methods and functions to lower camelCase; the code is currently inconsistent, but this matches Qt and better matches FFmpeg's function naming (snake_case) with upper CamelCase for types.) |
||
(49 intermediate revisions by 9 users not shown) | |||
Line 1: | Line 1: | ||
− | = | + | == Design guidelines == |
− | + | Here's some advice to keep in mind while designing or updating code: | |
− | For more of | + | * '''Make large style changes in a separate patch from functional changes.''' This makes it easier for us to review your changes, and to comb through the history looking for the commit that introduced a bug. |
+ | * '''Stay platform-independent.''' Use Qt instead of #ifdefs where possible. If you must add something that only works on one platform, add a compile-time flag so that it's only built where it works. See settings.pro for several examples. | ||
+ | * '''Platform support:''' Myth targets distro support for maintained releases at the following revisions: | ||
+ | ** Debian "stable" | ||
+ | ** Fedora "current" and "current-1" | ||
+ | ** Ubuntu "current" and "LTS" | ||
+ | * '''Avoid dependencies.''' Dependencies are added very rarely to Myth, so think carefully before you suggest a new item. If you want to link to a library that's small and has unique functionality, you may be able to get it added to the core Myth build. In general, though, avoid external dependencies if at all possible. See [[External libraries]] and [[Build from Source#Dependencies]]. | ||
+ | * '''Locking order''' | ||
+ | ** Most classes with more than one lock/mutex/semaphore will have a [[Locking_Order|locking order]] comment. You must follow this locking order to avoid A,B;B,A deadlocks. | ||
+ | ** If you create a class with more than one lock, document your locking order. | ||
+ | * '''Unused parameters:''' Functions that have to satisfy a given interface definition sometimes have parameters that are not used. | ||
+ | ** The name of an unused parameter should be put in a comment to document that the parameter is intentionally unused. | ||
+ | ** If the function does not have to follow such an interface definition the parameter should simply be removed. | ||
+ | ** If a function has multiple code paths because of conditional compilation and not all code paths use the variable, that variable should be annotated in the unused code path with 'Q_UNUSED(variable)' for C++ code or '(void)variable' for C code. | ||
+ | * It's a good idea to follow [https://google.github.io/styleguide/cppguide.html Google's C++ style guide] when they don't contradict our standards. | ||
+ | ** For example [https://google.github.io/styleguide/cppguide.html#Header_Files Header files], in particular: | ||
+ | *** '''Don't use forward declarations''' for external code, e.g. Qt, instead <code>#include</code> the necessary header files. <code><iosfwd></code> is allowed in headers, but is unlikely to be necessary. | ||
+ | **** Forward declarations are currently used in the code for MythTV classes, and their use is not discouraged at this time. | ||
+ | *** '''[https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Names and Order of Includes]''' | ||
+ | **** For MythTV headers not in the same directory as a given file: | ||
+ | ***** if the header is in the same library or program <code>#include "path/as/needed/header.h"</code> relative to the libmyth.../ or mythprogram/ directory, e.g. in <code>libmythtv/foo/bar.h</code> include <code>libmythtv/baz/example.h</code> as <code>"baz/example.h"</code>. | ||
+ | ***** if the header is in a different library or program <code>#include "path/as/needed/header.h"</code> relative to the libs/ (or programs/) directory, e.g. in <code>libmythtv/foo/bar.h</code> or <code>mythfrontend/foobar.cpp</code> include <code>libmythbase/baz1/example2.h</code> as <code>"libmythbase/baz1/example2.h"</code>. | ||
+ | ***** The reason to use quotes here is to enable the use of <code>-iquote</code> instead of <code>-I</code> to reduce the number of directories the preprocessor has to search to find the header files. [https://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html POSIX c99] only specifies <code>-I</code>, [https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html GCC] and [https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-iquote-directory clang] support <code>-iquote</code>. <!-- So do Intel's compilers, but I don't know if anyone uses them. --> | ||
+ | **** Use the following order (with a blank line between each): | ||
+ | ***** the corresponding .h in a .cpp file. | ||
+ | ****** This ensures the header is self-sufficient and compiles standalone. | ||
+ | ***** C system headers (e.g. POSIX) | ||
+ | ***** C++ standard library | ||
+ | ***** other external libraries | ||
+ | ***** Qt headers | ||
+ | ***** MythTV headers (sorted by library) | ||
+ | |||
+ | === C++ === | ||
+ | * '''Use C++17.''' (C++11 for fixes/31.) | ||
+ | ** Avoid outdated C constructs like TRUE/FALSE macros, C-style casting, NULL/0 instead of nullptr, etc. | ||
+ | ** Use c++11 brace initialization for member variables. | ||
+ | ** '''Don't''' use bare <code>enum</code>s. They pollute the namespace and have no type safety. Instead, use <code>enum class</code> or equivalently <code>enum struct</code>. | ||
+ | *** '''Do''' specify the type of enumerations, e.g. <code>enum class kType : int32_t</code>. | ||
+ | *** '''<code>enum</code>s are not flags.''' Bare <code>enum</code>s have no type safety and <code>enum class</code>es require static casts to an integer type. Prefer '''<code>[https://en.cppreference.com/w/cpp/utility/bitset const std::bitset]</code>s''' in a namespace, or alternatively [https://doc.qt.io/qt-5/qflags.html QFlags]. | ||
+ | **** Use bit shifts (i.e. <code>1U << n</code>) to define flags. This is much more readable than hexadecimal and easier to modify if necessary. | ||
+ | ** '''[https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros Avoid preprocessor macros]''' | ||
+ | *** for constants use <code>const</code> or <code>static const</code> or, for [https://en.cppreference.com/w/cpp/named_req/LiteralType literal types], use <code>constexpr</code> or <code>static constexpr</code>. | ||
+ | **** Use <code>static</code> except in header files at namespace scope, where they must be <code>inline</code>. | ||
+ | *** '''Don't''' use function macros: the preprocessor has no type safety and makes the code harder to read by obfuscating what the code actually is. '''Prefer inline functions''' instead. Also, please don't use hackery like [https://github.com/MythTV/mythtv/blob/dd172c0a0cda443539c5e91a9c2abb5c30bb671d/mythtv/libs/libmyth/programtypeflags.h this], which is completely unreadable. | ||
+ | * '''Encapsulate Well''' Make a Get method const. Return a copy and not a reference. Use Set methods, when defined in the header these entail no efficiency loss and allow you do add debugging code when necessary. | ||
+ | ** Make translation unit (file) -local functions <code>static</code> and constants <code>static const</code>. Or put them in an anonymous namespace, which must be used for file-local type definitions. This prevents them from being accessed from other files, which could create hard to diagnose bugs, and prevents name clashes from violating the One Definition Rule. | ||
+ | * '''Use private classes to keep <u>''popular''</u> header files stable.''' This design pattern is used in MythContext and others; most of the private data and methods are stored in a private class, defined in the .cpp file. A pointer to this class is stored as a member variable in the public class. This way, the private data can change without changing the header file, which prevents everyone from having to rebuild most of the source tree for a popular header like mythcontext.h. | ||
+ | ** '''Don't overuse this idiom.''' It hurts readability and the code is slower due to the extra indirection through the pointer. | ||
+ | ** [https://en.cppreference.com/w/cpp/language/pimpl This] is the [[Wikipedia:Opaque_pointer|PIMPL idiom]], a type of [[Wikipedia:Bridge_pattern|Bridge pattern]], and similar in some ways to a [[Wikipedia:Facade_pattern|Facade pattern]]. | ||
+ | ** See also https://herbsutter.com/gotw/_100/ | ||
+ | * '''Don't use assert() or abort()''' Crashing is never good. Use proper error handling in place of assert. This doesn't mean you can't use asserts during development, just that by the time the code is ready for commit there should be no need for them. | ||
+ | * '''Don't use C++ exceptions''' Unlike say Java, C++ doesn't have any way to tell the user of a method what exceptions must be caught, but gcc does let you mark a return value as something that must be examined. To deal with exceptions that can occur during object construction use either two part initialization or a static factory method that returns NULL when it fails. | ||
+ | * '''Don't use <code>using std::xxx;</code>''' Calling functions from the standard library with std::xxx() makes it obvious at the point of invocation that it's a standard library function, not a function from some other library. Then, there's no need to search through all the nested include files to see if somewhere (possibly 3 or 4 levels deep) there is a <code>using:xxx</code> statement. This also applies to any component from the standard library, such as types, e.g. <code>std::vector</code>. | ||
+ | ** '''Exception''': <code>using namespace std::(.*?_|)literals;</code> is explicitly '''allowed'''. | ||
− | == MythTV == | + | === Qt === |
+ | * '''Use [[MythUI]] for GUI tasks.''' We are currently working on a UI layer that's independent of Qt ([[MythUI]]), and patches that use native Qt widgets or the old libMyth code will be rejected. Use libmythui/* instead. | ||
+ | * '''Use Qt for non-GUI tasks.''' Don't design your own helper classes or use platform-dependent code when Qt already offers a class. Strings, containers (lists etc) and mutexes are good examples of Qt's generic functionality. | ||
+ | ** The current development version of MythTV requires Qt >=5.9, so check your calls against the [https://doc.qt.io/qt-5/index.html Qt 5 documentation] so you don't break things. Qt no longer supports old documentation snapshots, so you will need to look through the documentation to see if functions have been obsoleted. You should also set the QT_MIN_VERSION_STR variable in <code>configure</code> so that the compiler will warn about deprecated functions. | ||
+ | ** We have our own replacement for QProcess called MythSystem. QProcess has had an unfixed deadlocking bug in Qt 4.0, that was unfixed by version 4.7. More recent version have not been tested. | ||
+ | ** We have our own logging macro LOG, use this instead of the Qt debugging output macros and cerr. | ||
+ | * '''QString''' and other Qt container classes are not thread-safe. You can not use them in multiple threads without locking as you might a "const char*" string or a std::string. As of Qt4 assigned copies can be used in another thread, so something like this is now safe "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); return myString; }" | ||
+ | * '''Don't use Qt signals and slots in backend code''' There are a number of gotchas with signals and slots and we've decided to use them only in UI code as a result of dealing with all the hard to debug crashes that have resulted. | ||
+ | |||
+ | == MythTV Style == | ||
+ | |||
+ | The rules of thumb on this page are meant to help developers ensure that their code fits in with the conventions used in the MythTV source. The goal is to enhance readability. After your code becomes a part of MythTV many other hands are likely to touch it and they will be following these conventions so if you apply some other convention the code will over time become more difficult to read. Do not apply these conventions to externally maintained code, such as ffmpeg, try your best to follow their conventions and whenever possible get your changes to the external code applied upstream as soon as possible. | ||
=== Spaces and braces === | === Spaces and braces === | ||
Line 12: | Line 76: | ||
Indents are accomplished with 4 spaces. Please set your editor to convert tabs to spaces, or at least do the conversion before you submit a patch. | Indents are accomplished with 4 spaces. Please set your editor to convert tabs to spaces, or at least do the conversion before you submit a patch. | ||
+ | |||
+ | Do not leave trailing whitespace at the end of a line (including an empty line consisting of only whitespace); this clutters the diffs. Also, use Unix line endings (i.e. only line feed <code>'\n'</code>; if necessary, you can configure git to automatically convert between line endings. | ||
You should put 1 space between keywords like <code><nowiki>if</nowiki></code> or <code><nowiki>for</nowiki></code> and their parenthesized lists. By contrast, don't do the same for function names; there should be no space between the name and its opening parenthesis. There should be no spacing between parentheses and the items they contain. | You should put 1 space between keywords like <code><nowiki>if</nowiki></code> or <code><nowiki>for</nowiki></code> and their parenthesized lists. By contrast, don't do the same for function names; there should be no space between the name and its opening parenthesis. There should be no spacing between parentheses and the items they contain. | ||
− | Spaces are used between binary operators ( | + | Spaces are used between [[Wikipedia:Operators_in_C_and_C++|binary operators]] (e.g. <code><nowiki>+</nowiki></code> or <code><nowiki>=</nowiki></code>). |
− | Braces should go on their own line. The exception to this is | + | Braces should go on their own line. The exception to this is accessor methods defined inline as one line, if the line does not exceed 80 characters. One-line <code><nowiki>if</nowiki></code> or <code><nowiki>else</nowiki></code> bodies should have braces. Do not declare a variable of a struct, class, enum, or union as part of its definition; i.e. the line should be <code>};</code> only, with <code>Type var;</code> on a separate line. |
+ | |||
+ | If a boolean conditional expression is broken over multiple lines, placing the closing <code>)</code> on its own line helps separate the condition from subsequent code. | ||
Lines should not exceed 80 characters in width. If you have a long statement, indent the continuations so they line up vertically. Generally, if you indent past the last opening parenthesis of the previous line, you'll be in good shape. | Lines should not exceed 80 characters in width. If you have a long statement, indent the continuations so they line up vertically. Generally, if you indent past the last opening parenthesis of the previous line, you'll be in good shape. | ||
<pre><nowiki> | <pre><nowiki> | ||
− | + | enum my_enum { | |
kTestOne, ///< This represents blah | kTestOne, ///< This represents blah | ||
kTestTwo, ///< Widget Blah | kTestTwo, ///< Widget Blah | ||
kTestThree, ///< Blah Blah | kTestThree, ///< Blah Blah | ||
kTestSentinel, | kTestSentinel, | ||
− | } MyEnum; | + | }; |
+ | my_enum MyEnum; | ||
static inline void run_three_times(const QStringList &strings, const char *buffer) | static inline void run_three_times(const QStringList &strings, const char *buffer) | ||
Line 34: | Line 103: | ||
{ | { | ||
MyEnum result = my_func(i, strings); | MyEnum result = my_func(i, strings); | ||
− | if (kTestOne == result || kTestTwo == result) | + | |
+ | if (kTestOne == result || | ||
+ | kTestTwo == result | ||
+ | ) | ||
+ | { | ||
DoSomething(); | DoSomething(); | ||
+ | } | ||
else | else | ||
+ | { | ||
DoTheOpposite(); | DoTheOpposite(); | ||
+ | } | ||
result = my_other_func(result, strings); | result = my_other_func(result, strings); | ||
Line 43: | Line 119: | ||
{ | { | ||
if (DoSomething()) | if (DoSomething()) | ||
+ | { | ||
DoMore(); | DoMore(); | ||
+ | } | ||
} | } | ||
else | else | ||
Line 69: | Line 147: | ||
=== Class and variable names === | === Class and variable names === | ||
− | + | {| class="wikitable" | |
+ | ! Type !! Naming Convention | ||
+ | |- | ||
+ | | class<br>enum || [[Wikipedia:Camel case|CamelCase]] with initial capital letter | ||
+ | |- | ||
+ | | method<br>function || [[Wikipedia:Camel case|camelCase]] with initial lowercase letter | ||
+ | |- | ||
+ | | variable || all lowercase or [[Wikipedia:Camel case|camelCase]] (with initial lowercase letter) | ||
+ | |} | ||
+ | |||
+ | {| class="wikitable" | ||
+ | ! Variable type !! Name prefix | ||
+ | |- | ||
+ | | The few '''global''' variables || <code>g_</code> | ||
+ | |- | ||
+ | | constant || <code>k_</code> | ||
+ | |- | ||
+ | | '''static''' class member || <code>s_</code> | ||
+ | |- | ||
+ | | class member || <code>m_</code> | ||
+ | |- | ||
+ | | any other variable || must '''not''' start with regex <code>._</code> | ||
+ | |} | ||
+ | |||
+ | NB: identifiers at file scope beginning with regex <code>_[A-Z]</code> and all identifiers containing two consecutive underscores are reserved by the C and C++ standards. | ||
+ | |||
+ | Avoid naming variables such that both singular and plural are used, e.g. don't use both <code>attempt</code> and <code>attempts</code>. Using both makes the code hard to read, so choose a different, more descriptive name for one of the variables, e.g. <code>maxAttempts</code>, or if one of the variables is an iterator, you could use a generic iterator name, e.g. <code>i</code>. However, if the plural variable is a container, using the singular form as the iterator in a for each loop is acceptable. | ||
Make your variable names left-to-right specific. Related variables should sort together alphabetically; <code><nowiki>audioId</nowiki></code> and <code><nowiki>audioMutex</nowiki></code> should stand apart from <code><nowiki>videoTimeRemaining</nowiki></code>. The names should be compact, but be as self-explanatory as possible. | Make your variable names left-to-right specific. Related variables should sort together alphabetically; <code><nowiki>audioId</nowiki></code> and <code><nowiki>audioMutex</nowiki></code> should stand apart from <code><nowiki>videoTimeRemaining</nowiki></code>. The names should be compact, but be as self-explanatory as possible. | ||
Line 95: | Line 199: | ||
{ | { | ||
QMutexLocker locker(&m_lock); | QMutexLocker locker(&m_lock); | ||
+ | |||
return m_deviceName; | return m_deviceName; | ||
} | } | ||
Line 107: | Line 212: | ||
</nowiki></pre> | </nowiki></pre> | ||
− | + | == Comments == | |
Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory. Comments should give necessary overviews of functionality, or document strange cases, ranges, etc. | Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory. Comments should give necessary overviews of functionality, or document strange cases, ranges, etc. | ||
Line 113: | Line 218: | ||
Description of variables' use and purpose should accompany the declaration. For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given. | Description of variables' use and purpose should accompany the declaration. For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given. | ||
− | Doxygen comments are encouraged in the code. But keep them brief and don't use them when they don't add | + | Doxygen comments are encouraged in the code. But keep them brief and don't use them when they don't add information not already obvious from the variable or function name. Prefer the Javacdoc Doxygen syntax, i.e.: <code>/** ... */</code>, <code>@command</code>, and <code>///</code> for single line Doxygen comments. |
− | information not already obvious from the variable or function name. | + | |
+ | == Translation guidelines == | ||
+ | |||
+ | {{Note box|This section is still a work in progress. Please do not apply those guidelines or edit this section for now.}} | ||
+ | |||
+ | '''Dont's''' | ||
+ | |||
+ | * '''Never use QObject::tr() directly.''' You can use the tr() function of a class that inherits from QObject but do not directly call the QObject one... When you call QObject::tr() directly it puts all the strings to translate in a QObject "translation context" and this gives no clue to the translator as to what they are actually used for. There is also the possibility of translation clashes this way. | ||
+ | * '''Don't build a sentence by concatenating together many words.''' That might work fine for languages which have a phrase structure similar to English but not for the others. | ||
+ | * Avoid concatenating together many sentences unless they are separated by a carriage return. It's usually better to regroup all those sentences together and call the translation function on that group of sentences. | ||
+ | |||
+ | '''Do's''' | ||
+ | |||
+ | If your class inherits from QObject, you must use the Q_OBJECT macro in the class declaration (before you declare anything else). Please note that the class declaration should usually be in a .h header file. There are workarounds around this but it's preferable to avoid using them (it has to do with the way Qt adds, amongst other things, translation support). | ||
+ | |||
+ | If your class doesn't inherit from QObject, you need to call the Q_DECLARE_TR_FUNCTIONS macro to be able to use the tr() function from your class. As with the Q_OBJECT macro, it must be declared before anything else and you must provide it the class name (usually, it could be something else but that's not usually recommended). | ||
+ | |||
+ | e.g. | ||
+ | |||
+ | <pre><nowiki> | ||
+ | class AudioOutput | ||
+ | { | ||
+ | Q_DECLARE_TR_FUNCTIONS(AudioOutput) | ||
+ | |||
+ | public: | ||
+ | AudioOutput(const QString &device_name); | ||
+ | |||
+ | <snip> | ||
+ | }; | ||
+ | </nowiki></pre> | ||
+ | |||
+ | Once you have added the Q_OBJECT or Q_DECLARE_TR_FUNCTIONS macro you should be able to call the tr() function without any class prefix. | ||
+ | |||
+ | === Translation context === | ||
+ | |||
+ | What are those translation context we mention briefly earlier? | ||
+ | |||
+ | Usually, but it doesn't always have to be, it's the name of the class you call tr() from. It's part of the lookup key used to retrieve the translation from the translation files. | ||
+ | |||
+ | The lookup key used to retrieve the translations is composed of the translation context, the original untranslated string (in our case in US English) and, when provided, a disambiguation string (sometimes still called a comment in Qt's documentation but they are way more than that..). | ||
+ | |||
+ | For now, let's focus on strings which do not have disambiguation strings. | ||
+ | |||
+ | Let's take the following example | ||
+ | |||
+ | <pre><nowiki> | ||
+ | void FirstClass::init(void) | ||
+ | { | ||
+ | setLabel(tr("A String")); | ||
+ | } | ||
+ | |||
+ | void SecondClass::init(void) | ||
+ | { | ||
+ | setLabel(tr("A String")); | ||
+ | } | ||
+ | </nowiki></pre> | ||
+ | |||
+ | This will, under normal circumstances (there are ways around it if the class doesn't inherit from QObject), create two different strings, one in the "FirstClass" context the other in the "SecondClass" context and both will need to be translated separately. The translation editor will notice these are identical and offer the translator to use the translation of the first one filled in the fill in the translation of the second one). | ||
+ | |||
+ | If I want to look up the translation of translation of either one of those strings from another class we will have to specify their context (either "FirstClass" or "SecondClass" in this case) either in by prefixing tr with the class name or by specifying their context to a function that allows use to specify the translation context such as QCoreApplication::translate(). | ||
+ | |||
+ | === Disambiguation string === | ||
+ | |||
+ | It obviously couldn't be as simple as that.. | ||
+ | |||
+ | Just kidding... | ||
+ | |||
+ | When the translation context and the original untranslated string is not enough to differentiate one string from the other, the disambiguation string comes to the rescue. It gets added to the lookup key and must be provided to retrieve that translation. It used to be the old way of commenting translations and is still documented as such in quite a few places in Qt's documentation but don't be fooled, it becomes part of the lookup key and must be provided to lookup a translation which uses it. | ||
+ | |||
+ | This is usually how they are used: | ||
+ | |||
+ | <pre><nowiki> | ||
+ | void FirstClass::init(void) | ||
+ | { | ||
+ | setLabel(tr("A String")); | ||
+ | setCategory(tr("Unknown", "Category")); | ||
+ | setType(tr("Unknown", "Type")); | ||
+ | setUnknown(tr(Unknown")); | ||
+ | } | ||
+ | </nowiki></pre> | ||
+ | |||
+ | All three "Unknown" strings will be in the context "FirstClass" but will have their own translation. The first two will have a disambiguation string, the third one will have none. This could be used for example when the gender of the nouns to which these strings refer to could affect the translation ("Unknown" could be written differently in another language depending on whether it refers to a feminine or masculine noun). | ||
− | + | There is also a poorly documented (it appears to be only documented for the lower level (and which you should never have to use) QTranslator::translate function), the fallback to the string with no disambiguation string if the string with the same context, original string and disambiguation string is not present in the translation file. This could happen when the translation file is not up to date with the application or if one of the parameters passed to tr() (or the other translation functions) is a variable. We will discuss what to do when you have to pass variables to the translation functions later... | |
− | + | {{Note box|Never pass a '''null''' disambiguation string as parameter, if you need to provide one pass an empty string ("").}} | |
− | + | === But I lack class (or what to do when you are not in a member function) === | |
− | + | ||
− | + | Adding the tr() function using either the Q_OBJECT or Q_DECLARE_TR_FUNCTIONS macro only works for classes, a different approach must be used when your are not in a member function. | |
− | + | ||
− | + | If you have a mix of member functions (ie part of a class) and regular (C-like) functions (which is pretty common in the settings pages for example), you can call the tr() function of the class from your function. The translations done that way will share the same translation context as the class which is usually what you want when those functions are called from the member functions of that class. | |
− | + | ||
− | + | If you don't have any functions or at least any function you would like to share it's translation context with then you have to resort to using QCoreApplication::translate(). | |
− | + | ||
− | + | There are many ways to call it (more info will be added on them as I update this section) but in its simplest way it must be called like so | |
− | + | ||
− | + | <pre><nowiki> | |
− | + | QCoreApplication::translate("(My new translation context)", "The text I want translated") | |
− | + | ||
− | + | qApp->translate("(My new translation context)", "The text I want translated") | |
− | + | ||
+ | </nowiki></pre> | ||
+ | |||
+ | Both of these examples are equivalent, the first time we directly call translate from QCoreApplication and the second time we call it from QApplication which inherits it from QCoreApplication. qApp is actually a macro that return a pointer to the unique application object. QApplication is meant to be used with GUI applications so it's usually safer to call it from QCoreApplication. | ||
+ | |||
+ | In MythTV, any context which is not a class name should be put in parenthesis. It makes those "virtual" contexts easier to spot and avoid name clashes with contexts which are actual class names. | ||
+ | |||
+ | {{Note box|If you add any additional source code folders which contains strings which should be translated, please contact knightr on IRC or nriendeau (at) mythtv.org.}} | ||
− | == MythWeb == | + | == MythWeb Style == |
− | + | MythWeb is written in PHP and Javascript so a different style makes sense. | |
− | |||
=== Comment Style === | === Comment Style === |
Latest revision as of 22:35, 28 February 2022
Design guidelines
Here's some advice to keep in mind while designing or updating code:
- Make large style changes in a separate patch from functional changes. This makes it easier for us to review your changes, and to comb through the history looking for the commit that introduced a bug.
- Stay platform-independent. Use Qt instead of #ifdefs where possible. If you must add something that only works on one platform, add a compile-time flag so that it's only built where it works. See settings.pro for several examples.
- Platform support: Myth targets distro support for maintained releases at the following revisions:
- Debian "stable"
- Fedora "current" and "current-1"
- Ubuntu "current" and "LTS"
- Avoid dependencies. Dependencies are added very rarely to Myth, so think carefully before you suggest a new item. If you want to link to a library that's small and has unique functionality, you may be able to get it added to the core Myth build. In general, though, avoid external dependencies if at all possible. See External libraries and Build from Source#Dependencies.
- Locking order
- Most classes with more than one lock/mutex/semaphore will have a locking order comment. You must follow this locking order to avoid A,B;B,A deadlocks.
- If you create a class with more than one lock, document your locking order.
- Unused parameters: Functions that have to satisfy a given interface definition sometimes have parameters that are not used.
- The name of an unused parameter should be put in a comment to document that the parameter is intentionally unused.
- If the function does not have to follow such an interface definition the parameter should simply be removed.
- If a function has multiple code paths because of conditional compilation and not all code paths use the variable, that variable should be annotated in the unused code path with 'Q_UNUSED(variable)' for C++ code or '(void)variable' for C code.
- It's a good idea to follow Google's C++ style guide when they don't contradict our standards.
- For example Header files, in particular:
- Don't use forward declarations for external code, e.g. Qt, instead
#include
the necessary header files.<iosfwd>
is allowed in headers, but is unlikely to be necessary.- Forward declarations are currently used in the code for MythTV classes, and their use is not discouraged at this time.
- Names and Order of Includes
- For MythTV headers not in the same directory as a given file:
- if the header is in the same library or program
#include "path/as/needed/header.h"
relative to the libmyth.../ or mythprogram/ directory, e.g. inlibmythtv/foo/bar.h
includelibmythtv/baz/example.h
as"baz/example.h"
. - if the header is in a different library or program
#include "path/as/needed/header.h"
relative to the libs/ (or programs/) directory, e.g. inlibmythtv/foo/bar.h
ormythfrontend/foobar.cpp
includelibmythbase/baz1/example2.h
as"libmythbase/baz1/example2.h"
. - The reason to use quotes here is to enable the use of
-iquote
instead of-I
to reduce the number of directories the preprocessor has to search to find the header files. POSIX c99 only specifies-I
, GCC and clang support-iquote
.
- if the header is in the same library or program
- Use the following order (with a blank line between each):
- the corresponding .h in a .cpp file.
- This ensures the header is self-sufficient and compiles standalone.
- C system headers (e.g. POSIX)
- C++ standard library
- other external libraries
- Qt headers
- MythTV headers (sorted by library)
- the corresponding .h in a .cpp file.
- For MythTV headers not in the same directory as a given file:
- Don't use forward declarations for external code, e.g. Qt, instead
- For example Header files, in particular:
C++
- Use C++17. (C++11 for fixes/31.)
- Avoid outdated C constructs like TRUE/FALSE macros, C-style casting, NULL/0 instead of nullptr, etc.
- Use c++11 brace initialization for member variables.
- Don't use bare
enum
s. They pollute the namespace and have no type safety. Instead, useenum class
or equivalentlyenum struct
.- Do specify the type of enumerations, e.g.
enum class kType : int32_t
. -
enum
s are not flags. Bareenum
s have no type safety andenum class
es require static casts to an integer type. Preferconst std::bitset
s in a namespace, or alternatively QFlags.- Use bit shifts (i.e.
1U << n
) to define flags. This is much more readable than hexadecimal and easier to modify if necessary.
- Use bit shifts (i.e.
- Do specify the type of enumerations, e.g.
- Avoid preprocessor macros
- for constants use
const
orstatic const
or, for literal types, useconstexpr
orstatic constexpr
.- Use
static
except in header files at namespace scope, where they must beinline
.
- Use
- Don't use function macros: the preprocessor has no type safety and makes the code harder to read by obfuscating what the code actually is. Prefer inline functions instead. Also, please don't use hackery like this, which is completely unreadable.
- for constants use
- Encapsulate Well Make a Get method const. Return a copy and not a reference. Use Set methods, when defined in the header these entail no efficiency loss and allow you do add debugging code when necessary.
- Make translation unit (file) -local functions
static
and constantsstatic const
. Or put them in an anonymous namespace, which must be used for file-local type definitions. This prevents them from being accessed from other files, which could create hard to diagnose bugs, and prevents name clashes from violating the One Definition Rule.
- Make translation unit (file) -local functions
- Use private classes to keep popular header files stable. This design pattern is used in MythContext and others; most of the private data and methods are stored in a private class, defined in the .cpp file. A pointer to this class is stored as a member variable in the public class. This way, the private data can change without changing the header file, which prevents everyone from having to rebuild most of the source tree for a popular header like mythcontext.h.
- Don't overuse this idiom. It hurts readability and the code is slower due to the extra indirection through the pointer.
- This is the PIMPL idiom, a type of Bridge pattern, and similar in some ways to a Facade pattern.
- See also https://herbsutter.com/gotw/_100/
- Don't use assert() or abort() Crashing is never good. Use proper error handling in place of assert. This doesn't mean you can't use asserts during development, just that by the time the code is ready for commit there should be no need for them.
- Don't use C++ exceptions Unlike say Java, C++ doesn't have any way to tell the user of a method what exceptions must be caught, but gcc does let you mark a return value as something that must be examined. To deal with exceptions that can occur during object construction use either two part initialization or a static factory method that returns NULL when it fails.
- Don't use
using std::xxx;
Calling functions from the standard library with std::xxx() makes it obvious at the point of invocation that it's a standard library function, not a function from some other library. Then, there's no need to search through all the nested include files to see if somewhere (possibly 3 or 4 levels deep) there is ausing:xxx
statement. This also applies to any component from the standard library, such as types, e.g.std::vector
.- Exception:
using namespace std::(.*?_|)literals;
is explicitly allowed.
- Exception:
Qt
- Use MythUI for GUI tasks. We are currently working on a UI layer that's independent of Qt (MythUI), and patches that use native Qt widgets or the old libMyth code will be rejected. Use libmythui/* instead.
- Use Qt for non-GUI tasks. Don't design your own helper classes or use platform-dependent code when Qt already offers a class. Strings, containers (lists etc) and mutexes are good examples of Qt's generic functionality.
- The current development version of MythTV requires Qt >=5.9, so check your calls against the Qt 5 documentation so you don't break things. Qt no longer supports old documentation snapshots, so you will need to look through the documentation to see if functions have been obsoleted. You should also set the QT_MIN_VERSION_STR variable in
configure
so that the compiler will warn about deprecated functions. - We have our own replacement for QProcess called MythSystem. QProcess has had an unfixed deadlocking bug in Qt 4.0, that was unfixed by version 4.7. More recent version have not been tested.
- We have our own logging macro LOG, use this instead of the Qt debugging output macros and cerr.
- The current development version of MythTV requires Qt >=5.9, so check your calls against the Qt 5 documentation so you don't break things. Qt no longer supports old documentation snapshots, so you will need to look through the documentation to see if functions have been obsoleted. You should also set the QT_MIN_VERSION_STR variable in
- QString and other Qt container classes are not thread-safe. You can not use them in multiple threads without locking as you might a "const char*" string or a std::string. As of Qt4 assigned copies can be used in another thread, so something like this is now safe "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); return myString; }"
- Don't use Qt signals and slots in backend code There are a number of gotchas with signals and slots and we've decided to use them only in UI code as a result of dealing with all the hard to debug crashes that have resulted.
MythTV Style
The rules of thumb on this page are meant to help developers ensure that their code fits in with the conventions used in the MythTV source. The goal is to enhance readability. After your code becomes a part of MythTV many other hands are likely to touch it and they will be following these conventions so if you apply some other convention the code will over time become more difficult to read. Do not apply these conventions to externally maintained code, such as ffmpeg, try your best to follow their conventions and whenever possible get your changes to the external code applied upstream as soon as possible.
Spaces and braces
The one hard and fast rule of Myth development: don't use tab characters.
Indents are accomplished with 4 spaces. Please set your editor to convert tabs to spaces, or at least do the conversion before you submit a patch.
Do not leave trailing whitespace at the end of a line (including an empty line consisting of only whitespace); this clutters the diffs. Also, use Unix line endings (i.e. only line feed '\n'
; if necessary, you can configure git to automatically convert between line endings.
You should put 1 space between keywords like if
or for
and their parenthesized lists. By contrast, don't do the same for function names; there should be no space between the name and its opening parenthesis. There should be no spacing between parentheses and the items they contain.
Spaces are used between binary operators (e.g. +
or =
).
Braces should go on their own line. The exception to this is accessor methods defined inline as one line, if the line does not exceed 80 characters. One-line if
or else
bodies should have braces. Do not declare a variable of a struct, class, enum, or union as part of its definition; i.e. the line should be };
only, with Type var;
on a separate line.
If a boolean conditional expression is broken over multiple lines, placing the closing )
on its own line helps separate the condition from subsequent code.
Lines should not exceed 80 characters in width. If you have a long statement, indent the continuations so they line up vertically. Generally, if you indent past the last opening parenthesis of the previous line, you'll be in good shape.
enum my_enum { kTestOne, ///< This represents blah kTestTwo, ///< Widget Blah kTestThree, ///< Blah Blah kTestSentinel, }; my_enum MyEnum; static inline void run_three_times(const QStringList &strings, const char *buffer) { for (int i = 0; i < 3; i++) { MyEnum result = my_func(i, strings); if (kTestOne == result || kTestTwo == result ) { DoSomething(); } else { DoTheOpposite(); } result = my_other_func(result, strings); if (kTestOne == result || kTestTwo == result) { if (DoSomething()) { DoMore(); } } else { DoTheOpposite(); } } }
Note: There are some things above that are not required by the coding standard. Using result == kTestOne is equally valid as kTestOne == result. The reason some developers prefer it is because it avoids introducing a "=" typo bug. If you run across the variation you don't prefer, leave it alone.
For Vi(m) users: the following options can be put into your ~/.vimrc file -
set expandtab set tabstop=4 set shiftwidth=4
And if it's not too intrusive, this line can appear in the source code, and Vim will apply the correct settings:
/* vim: set expandtab tabstop=4 shiftwidth=4: */
Class and variable names
Type | Naming Convention |
---|---|
class enum |
CamelCase with initial capital letter |
method function |
camelCase with initial lowercase letter |
variable | all lowercase or camelCase (with initial lowercase letter) |
Variable type | Name prefix |
---|---|
The few global variables | g_
|
constant | k_
|
static class member | s_
|
class member | m_
|
any other variable | must not start with regex ._
|
NB: identifiers at file scope beginning with regex _[A-Z]
and all identifiers containing two consecutive underscores are reserved by the C and C++ standards.
Avoid naming variables such that both singular and plural are used, e.g. don't use both attempt
and attempts
. Using both makes the code hard to read, so choose a different, more descriptive name for one of the variables, e.g. maxAttempts
, or if one of the variables is an iterator, you could use a generic iterator name, e.g. i
. However, if the plural variable is a container, using the singular form as the iterator in a for each loop is acceptable.
Make your variable names left-to-right specific. Related variables should sort together alphabetically; audioId
and audioMutex
should stand apart from videoTimeRemaining
. The names should be compact, but be as self-explanatory as possible.
enum AudioChannelEnum { kChannelLeft = 0x01, kChannelRight = 0x02, kChannelCenter = 0x04, kChannelLeftBack = 0x08, kChannelRightBack = 0x10, kChannelBase = 0x20, kChannelAll = 0xff } AudioChannels; class AudioOutput { public: AudioOutput(const QString &device_name); /// Adjust the volume by a delta, from +100 to -100. void AdjustVolume(int change); /// \return Device name QString GetDeviceName(void) const { QMutexLocker locker(&m_lock); return m_deviceName; } protected: mutable QMutex m_lock; // This is mutable so that /// The absolute volume, from 0-100. int m_volume; // protected by m_lock QString m_deviceName; // protected by m_lock const static int kAudioBuffers; };
Comments
Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory. Comments should give necessary overviews of functionality, or document strange cases, ranges, etc.
Description of variables' use and purpose should accompany the declaration. For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given.
Doxygen comments are encouraged in the code. But keep them brief and don't use them when they don't add information not already obvious from the variable or function name. Prefer the Javacdoc Doxygen syntax, i.e.: /** ... */
, @command
, and ///
for single line Doxygen comments.
Translation guidelines
Note: This section is still a work in progress. Please do not apply those guidelines or edit this section for now.
Dont's
- Never use QObject::tr() directly. You can use the tr() function of a class that inherits from QObject but do not directly call the QObject one... When you call QObject::tr() directly it puts all the strings to translate in a QObject "translation context" and this gives no clue to the translator as to what they are actually used for. There is also the possibility of translation clashes this way.
- Don't build a sentence by concatenating together many words. That might work fine for languages which have a phrase structure similar to English but not for the others.
- Avoid concatenating together many sentences unless they are separated by a carriage return. It's usually better to regroup all those sentences together and call the translation function on that group of sentences.
Do's
If your class inherits from QObject, you must use the Q_OBJECT macro in the class declaration (before you declare anything else). Please note that the class declaration should usually be in a .h header file. There are workarounds around this but it's preferable to avoid using them (it has to do with the way Qt adds, amongst other things, translation support).
If your class doesn't inherit from QObject, you need to call the Q_DECLARE_TR_FUNCTIONS macro to be able to use the tr() function from your class. As with the Q_OBJECT macro, it must be declared before anything else and you must provide it the class name (usually, it could be something else but that's not usually recommended).
e.g.
class AudioOutput { Q_DECLARE_TR_FUNCTIONS(AudioOutput) public: AudioOutput(const QString &device_name); <snip> };
Once you have added the Q_OBJECT or Q_DECLARE_TR_FUNCTIONS macro you should be able to call the tr() function without any class prefix.
Translation context
What are those translation context we mention briefly earlier?
Usually, but it doesn't always have to be, it's the name of the class you call tr() from. It's part of the lookup key used to retrieve the translation from the translation files.
The lookup key used to retrieve the translations is composed of the translation context, the original untranslated string (in our case in US English) and, when provided, a disambiguation string (sometimes still called a comment in Qt's documentation but they are way more than that..).
For now, let's focus on strings which do not have disambiguation strings.
Let's take the following example
void FirstClass::init(void) { setLabel(tr("A String")); } void SecondClass::init(void) { setLabel(tr("A String")); }
This will, under normal circumstances (there are ways around it if the class doesn't inherit from QObject), create two different strings, one in the "FirstClass" context the other in the "SecondClass" context and both will need to be translated separately. The translation editor will notice these are identical and offer the translator to use the translation of the first one filled in the fill in the translation of the second one).
If I want to look up the translation of translation of either one of those strings from another class we will have to specify their context (either "FirstClass" or "SecondClass" in this case) either in by prefixing tr with the class name or by specifying their context to a function that allows use to specify the translation context such as QCoreApplication::translate().
Disambiguation string
It obviously couldn't be as simple as that..
Just kidding...
When the translation context and the original untranslated string is not enough to differentiate one string from the other, the disambiguation string comes to the rescue. It gets added to the lookup key and must be provided to retrieve that translation. It used to be the old way of commenting translations and is still documented as such in quite a few places in Qt's documentation but don't be fooled, it becomes part of the lookup key and must be provided to lookup a translation which uses it.
This is usually how they are used:
void FirstClass::init(void) { setLabel(tr("A String")); setCategory(tr("Unknown", "Category")); setType(tr("Unknown", "Type")); setUnknown(tr(Unknown")); }
All three "Unknown" strings will be in the context "FirstClass" but will have their own translation. The first two will have a disambiguation string, the third one will have none. This could be used for example when the gender of the nouns to which these strings refer to could affect the translation ("Unknown" could be written differently in another language depending on whether it refers to a feminine or masculine noun).
There is also a poorly documented (it appears to be only documented for the lower level (and which you should never have to use) QTranslator::translate function), the fallback to the string with no disambiguation string if the string with the same context, original string and disambiguation string is not present in the translation file. This could happen when the translation file is not up to date with the application or if one of the parameters passed to tr() (or the other translation functions) is a variable. We will discuss what to do when you have to pass variables to the translation functions later...
Note: Never pass a null disambiguation string as parameter, if you need to provide one pass an empty string ("").
But I lack class (or what to do when you are not in a member function)
Adding the tr() function using either the Q_OBJECT or Q_DECLARE_TR_FUNCTIONS macro only works for classes, a different approach must be used when your are not in a member function.
If you have a mix of member functions (ie part of a class) and regular (C-like) functions (which is pretty common in the settings pages for example), you can call the tr() function of the class from your function. The translations done that way will share the same translation context as the class which is usually what you want when those functions are called from the member functions of that class.
If you don't have any functions or at least any function you would like to share it's translation context with then you have to resort to using QCoreApplication::translate().
There are many ways to call it (more info will be added on them as I update this section) but in its simplest way it must be called like so
QCoreApplication::translate("(My new translation context)", "The text I want translated") qApp->translate("(My new translation context)", "The text I want translated")
Both of these examples are equivalent, the first time we directly call translate from QCoreApplication and the second time we call it from QApplication which inherits it from QCoreApplication. qApp is actually a macro that return a pointer to the unique application object. QApplication is meant to be used with GUI applications so it's usually safer to call it from QCoreApplication.
In MythTV, any context which is not a class name should be put in parenthesis. It makes those "virtual" contexts easier to spot and avoid name clashes with contexts which are actual class names.
Note: If you add any additional source code folders which contains strings which should be translated, please contact knightr on IRC or nriendeau (at) mythtv.org.
MythWeb Style
MythWeb is written in PHP and Javascript so a different style makes sense.
Comment Style
Outdented, meaning one indent level less then the code it's talking about, and above the code block, not below.
White Space Style
Soft Tab style (spaces, not tabs, and 4 spaces to a tabstop)
Classes
* Autoload * Multiton pattern