diff --git a/rules.org b/rules.org index 59b947f02839426a063432aded97ce4be5409513..8740439eee2fc4300435460fa1733689f4fac9f2 100644 --- a/rules.org +++ b/rules.org @@ -1,7 +1,7 @@ #+MACRO: version 0.7 #+LaTeX_HEADER: \def\rulesversion{0.7} #+TITLE: ATLAS C++ coding guidelines, version {{{version}}} -#+AUTHOR: Shaun Roe (CERN), Scott Snyder (BNL), and the former ATLAS Quality Control group +#+AUTHOR: Scott Snyder (BNL), Shaun Roe (CERN), and the former ATLAS Quality Control group #+EMAIL: Correspondence to snyder@bnl.gov. # Checked with org-mode 9.1.9 (as bundled with emacs 26.2) # To export to PDF use `org-latex-export-to-pdf'. @@ -63,7 +63,7 @@ This document is derived from the original ATLAS C++ coding standard, derived from work done by the CERN ``Project support team'' and SPIDER project, as documented in [[http://pst.web.cern.ch/PST/HandBookWorkBook/Handbook/Programming/CodingStandard/c++standard.pdf][CERN-UCO/1999/207]] [fn:PST99]. These previous guidelines have been significantly revised -to take into account the evolution of the C++ language [fn:Cxx14], +to take into account the evolution of the C++ language [fn:Cxx17], current practices in ATLAS, and the experience gained over the past decade. @@ -440,8 +440,8 @@ vector<int> x; // Missing std! Otherwise, the =using= may serve to hide problems with missing namespace qualifications in the headers. - Starting with C++11, =using= can also be used in ways similar to =typedef=. - Such usage is not covered by this rule. + This rule does not apply when =using= is used to define a new name + for a type (similarly to =typedef=). ** Control flow @@ -455,8 +455,8 @@ vector<int> x; // Missing std! - *Prefer range-based for loops.* [<<prefer-range-based-for>>] - C++11 introduced the `range-based for'. Prefer using this to a loop - with explicit iterators; that is, prefer: + Prefer a range-based for to a loop + with explicit iterators. That is, prefer: #+BEGIN_EXAMPLE std::vector<int> v = ...; for (int x : v) { @@ -499,22 +499,23 @@ for (std::vector<int>::const_iterator it = v.begin(); If you must ``fall through'' from one switch clause to another (excluding the trivial case of a clause with no statements), - this must be explicitly stated in a comment. This should, - however, be a rare case. + this should be explicitly indicated using the =fallthough= attribute. + This should, however, be a rare case. #+BEGIN_EXAMPLE switch (case) { case 1: doSomething(); - /* FALLTHROUGH */ + [[fallthrough]]; case 2: doSomethingMore(); break; ... #+END_EXAMPLE - gcc7 will warn about such constructs unless you use a comment like - in the example above. (C++17 will add a =fallthrough= attribute.) + Recent compilers will warn about such constructs unless you use + the attribute or a special comment. For new code, using the + attribute is preferred. - *An if-statement which does not fit in one line must have braces @@ -674,7 +675,7 @@ int* ip, jp; - *Be conservative in using* =auto=. [<<using-auto>>] - C++11 includes the new =auto= keyword, which allows one to omit explicitly + The =auto= keyword allows one to omit explicitly writing types that the compile can deduce. Examples: #+BEGIN_EXAMPLE @@ -706,6 +707,28 @@ auto foo = doSomething(); foo->doSomethingElse(); #+END_EXAMPLE + =auto= has also been observed to be a frequent source of errors + leading to unwanted copies of objects. For example, in this code: + + #+BEGIN_EXAMPLE +std::vector<std::vector<int> > arr = ...; +for (auto v : arr) { + for (auto elt : v) { ... +#+END_EXAMPLE + + each element of the outermost vector will be copied, as the assignment + to =v= will be done by value. One would probably want: + + #+BEGIN_EXAMPLE +std::vector<std::vector<int> > arr = ...; +for (const auto& v : arr) { + for (auto elt : v) { ... +#+END_EXAMPLE + + but having to be aware of the type like this kind of obviates the + motivation for using =auto= in the first place. Using the type + explicitly makes this sort of error much more difficult. + The current recommendation is to generally not use =auto= in place of a (possibly-qualified) simple type: @@ -925,8 +948,9 @@ public: }; #+END_EXAMPLE - This syntax was new in C++11. In C++98, this was achieved + In older versions of the language, this was achieved by declaring the deleted methods as private (and not implementing them). + For new code, prefer explicitly deleting the functions. #+BEGIN_EXAMPLE // There is only one ATLASExperimentalHall, @@ -1257,7 +1281,7 @@ void foo (std::unique_ptr<Object> obj); Here are a some additional examples to illustrate this. Assume that class =C= contains a member =Foo* m_owning_pointer= - which the class deletes. (In C++11, it would of course usually be better for + which the class deletes. (In modern C++, it would of course usually be better for this to be a =unique_ptr=.) #+BEGIN_EXAMPLE // --- Best @@ -1327,7 +1351,8 @@ std::unique_ptr<Foo> makeFoo() } // Ok if documented -// makeFoo() returns a newly-allocated Foo; caller must delete it. +// makeFoo() returns a newly-allocated Foo; +// caller must delete it. Foo* makeFoo() { return new Foo (...); @@ -1499,7 +1524,7 @@ void foo (std::unique_ptr<C> ptr); ... std::unique_ptr<C> p (new C); ... - foo (p); + foo (std::move (p)); // The argument of foo() is initialized by move. // p is left as a null pointer. #+END_EXAMPLE @@ -1985,6 +2010,7 @@ private: =MsgStream=. No production code should use =cout=. Classes which are not Athena-aware could use =cerr= before throwing an exception, but all Athena-aware classes should use =MSG::FATAL= and/or throw an exception. + In addition, it is acceptible to use writes to =cout= in unit tests. When using =MsgStream=, note that a call to, e.g., ~msg() << MSG::VERBOSE~ that is suppressed by the output level has a higher @@ -1994,7 +2020,6 @@ private: statements and are preferred in general for two reasons: they take up less space in the source code and indicate immediately that the message is correctly handled. - - *Check for all errors reported from functions.* [<<check-return-status>>] @@ -2064,12 +2089,11 @@ public: to check, at runtime, that a function did not throw any but a restricted set of exceptions. - Experience has shown that exception specifications are generally not useful, - and they have been deprecated in C++11 [fn:Sutter02]. They should not - be used in new code. In C++17, the use of non-empty exception specifications - is an error. + Experience has shown that exception specifications are generally not useful + and non-empty exception specifications are now an error [fn:Sutter02]. + They should not be used in new code. - C++14 added a new =noexcept= keyword. However, the motivation for this was + There is also the keyword =noexcept=. The motivation for this was really to address a specific problem with move constructors and exception-safety, and it is not clear that it is generally useful [fn:Krzemienski14]. @@ -2161,6 +2185,7 @@ Exception: std::exception Exception: Mine! #+END_EXAMPLE + Recent versions of gcc will warn about this. ** Parts of C++ to avoid @@ -2258,7 +2283,7 @@ void error(int severity, ...) // "severity" followed by a // NOT recommended to have function-like macro #define SQUARE(x) x*x -Better to define an inline function: +// Better to define an inline function: inline int square(int x) { return x*x; }; @@ -2422,14 +2447,7 @@ some machines, left-to-right on others. -- end note ] have long been able to do a good job of assigning values to registers; this is anyway highly-machine dependent. - The =register= keyword is ignored by all modern compilers, and has been - deprecated in the C++ standard for some time. As of C++17, - using the =register= keyword is an error. - - If you absolutely need to assign a variable to a register, many compilers - have a way of doing this using inline assembly or a related facility. - This is, however, inherently compiler- and machine-dependent, and would - only be useful in very special cases. + Use of the =register= keyword now an error. ** Readability and maintainability @@ -2547,12 +2565,12 @@ float ip_cut = 0.1 * Gaudi::Units::cm; ** Portability - - *All code must comply with the 2014 version of the ISO C++ standard (C++14)*. [<<standard-cxx>>] + - *All code must comply with the 2017 version of the ISO C++ standard (C++17)*. [<<standard-cxx>>] A draft of the standard which is essentially identical to the final version - may be found at [fn:Cxx14]. + may be found at [fn:Cxx17]. - At some point, compatibility with C++17 will also be required. + At some point, compatibility with C++20 will also be required. - *Make non-portable code easy to find and replace.* [<<limit-non-portable-code>>] @@ -2587,8 +2605,8 @@ float ip_cut = 0.1 * Gaudi::Units::cm; fail elsewhere. The ATLAS convention is to include the package name followed by the file name. - Watch out: if you list the package name twice, compilation will work - with CMT, but it may not work with other build systems. + Watch out: list the package name twice is wrong, but some build systems + don't catch it. #+BEGIN_EXAMPLE #include "/afs/cern.ch/atlas/software/dist/1.2.1/Foo/Bar/Qux.h" @@ -2987,7 +3005,9 @@ The comment includes the fact that it is the perpendicular distance. * Changes ** Version 0.7 - - Allow omitting the =default= clause in a =swtich= statement on an =enum= + - Minor cleanups and updates to take into acount that we now require C++17. + - Use the =fallthough= attribute, not a comment. + - Allow omitting the =default= clause in a =switch= statement on an =enum= that handles all possible values. Recent compilers will warn if some values are not handled, and it's better to get such a diagnosic at compile-time rather than at runtime. @@ -3034,7 +3054,7 @@ The comment includes the fact that it is the perpendicular distance. [fn:GOF] E. Gamma, R. Helm, R. Johnson, and J. Vlissides, /Design Patterns, Elements of Reusable Object-Oriented Software/, Addison-Wesley. [fn:Sutter02] [[http://www.gotw.ca/publications/mill22.htm][H. Sutter, /C++ Users Journal/, *20* (7), 2002.]] [fn:Krzemienski14] [[http://akrzemi1.wordpress.com/2014/04/24/noexcept-what-for/][A. Krzemie\nacute{}ski, /noexcept --- what for?/, 2014.]] -[fn:Cxx14] [[https://github.com/cplusplus/draft/blob/master/papers/n4140.pdf][ /Standard for the Programming Language C++/, n4140.]] +[fn:Cxx17] [[https://github.com/cplusplus/draft/blob/master/papers/n4659.pdf][ /Standard for the Programming Language C++/, n4659.]] [fn:Warnings] [[https://twiki.cern.ch/twiki/bin/view/AtlasComputing/FaqCompileTimeWarnings][FaqCompileTimeWarnings ATLAS wiki page.]] [fn:Doxygen] [[https://twiki.cern.ch/twiki/bin/view/AtlasComputing/DoxygenDocumentation][DoxygenDocumentation ATLAS wiki page.]] @@ -3148,5 +3168,5 @@ Doxygen also knows about //!< , I think? # eval: (defun org-html-target (target contents info) (let ((id (org-export-solidify-link-text (org-element-property :value target)))) (org-html--anchor id id))) # eval: (setq org-export-with-email t) # eval: (setq org-latex-prefer-user-labels t) -# eval: (define-advice org-latex--text-markup (:filter-return (text) brocket-fix) (if (string= (substring text 0 8) "\\texttt{") (replace-regexp-in-string "<<" "<{}<" text) text)) ; fix bad formatting of << +# eval: (define-advice org-latex--text-markup (:filter-return (text) brocket-fix) (if (string= (substring text 0 8) "\\texttt{") (replace-regexp-in-string "<<" "<{}<" (replace-regexp-in-string ">>" ">{}>" text)) text)) ; fix bad formatting of << and >> # End: