Skip to content

GitLab

Commit abb62a6e authored by scott snyder's avatar scott snyder
Browse files

Minor cleanups and updates to take into acount that we now require C++17.

parent d17c9f5e
Hide whitespace changes
Inline Side-by-side
Showing with 59 additions and 39 deletions
rules.org
View file @ abb62a6e
#+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:
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment