rules.org 110 KB
Newer Older
 scott snyder committed Sep 17, 2019 1 2 #+MACRO: version 0.7 #+LaTeX_HEADER: \def\rulesversion{0.7}  scott snyder committed Aug 23, 2017 3 #+TITLE: ATLAS C++ coding guidelines, version {{{version}}}  scott snyder committed Sep 17, 2019 4 #+AUTHOR: Scott Snyder (BNL), Shaun Roe (CERN), and the former ATLAS Quality Control group  scott snyder committed Aug 23, 2017 5 #+EMAIL: Correspondence to snyder@bnl.gov.  scott snyder committed Sep 17, 2019 6 # Checked with org-mode 9.1.9 (as bundled with emacs 26.2)  scott snyder committed Sep 17, 2019 7 # To export to PDF use org-latex-export-to-pdf'.  scott snyder committed Sep 18, 2019 8 9 # To export to HTML use org-html-export-to-html'. # To export to TWiki, load ox-twiki.el and use org-twiki-export-as-twiki.  scott snyder committed Aug 09, 2017 10   scott snyder committed Aug 23, 2017 11 # Along with the change to org-export-dictionary below, this changes  scott snyder committed Aug 09, 2017 12 13 14 # the Footnotes' section in HTML to References' #+LANGUAGE: en-references  scott snyder committed Dec 20, 2017 15 16 #+TWIKI_HEADER: *Do not edit this page. Its contents are generated automatically from the source at [[http://gitlab.cern.ch/ssnyder/coding-rules]].*  scott snyder committed Aug 09, 2017 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 # Put a frame around examples in LaTeX. #+LaTeX_HEADER: \usepackage{fancyvrb} #+LaTeX_HEADER: \RecustomVerbatimEnvironment{verbatim}{Verbatim}{frame=single} # For LaTeX, change footnotes to endnotes, and format markers like [n]'. # In HTML, the marker style is changed with the org-export-html-footnote-format # setting below. #+LaTeX_HEADER: \usepackage{endnotes} #+LaTeX_HEADER: \let\footnote=\endnote #+LaTeX_HEADER: \def\makeenmark{[\theenmark]} #+LaTeX_HEADER: \def\notesname{References} #+LaTeX_HEADER: \let\enoteformatsave=\enoteformat #+LaTeX_HEADER: \def\enoteformat{\enoteformatsave~} #+LaTeX_HEADER: \makeatletter #+LaTeX_HEADER: \def\@makefnmark{[\@thefnmark]} #+LaTeX_HEADER: \makeatother #+LaTeX_HEADER: \usepackage{lineno} #+LaTeX_HEADER: \linenumbers #+LaTeX_HEADER: \usepackage{fancyhdr} #+LaTeX_HEADER: \pagestyle{fancy}  scott snyder committed Aug 23, 2017 39 40 41 #+LaTeX_HEADER: \rfoot{Version \rulesversion} #+LaTeX_HEADER: \def\mylabel#1{\label{#1}#1}  scott snyder committed Aug 09, 2017 42 43 44 45 46 47 48 49 50 51 52 53 54  * Introduction This note gives a set of guidelines and recommendations for coding in C++ for the ATLAS experiment. There are several reasons for maintaining and following a set of programming guidelines. First, by following some rules, one can avoid some common errors and pitfalls in C++ programming, and thus have more reliable code. But even more important: a computer program should not only tell the machine what to do, but it should also tell /other people/ what you want the machine to do. (For much more elaboration on this idea,  scott snyder committed Dec 20, 2017 55 look up references on literate programming,'' such as [fn:Knuth84].)  scott snyder committed Aug 09, 2017 56 57 58 59 60 61 62 63 This is obviously important any time when you have many people working on a given piece of software, and such considerations would naturally lead to code that is easy to read and understand. Think of writing ATLAS code as another form of publication, and take the same care as you would writing up an analysis for colleagues. This document is derived from the original ATLAS C++ coding standard,  scott snyder committed Dec 20, 2017 64 [[https://cds.cern.ch/record/685315][ATL-SOFT-2002-001]] [fn:Atlas02], which was last revised in 2003. This itself  scott snyder committed Aug 09, 2017 65 derived from work done by the CERN Project support team''  scott snyder committed Dec 20, 2017 66 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].  scott snyder committed Aug 09, 2017 67 These previous guidelines have been significantly revised  scott snyder committed Sep 17, 2019 68 to take into account the evolution of the C++ language [fn:Cxx17],  scott snyder committed Aug 09, 2017 69 70 71 72 current practices in ATLAS, and the experience gained over the past decade. Some additional useful information on C++ programming may be  scott snyder committed Dec 20, 2017 73 found in [fn:Meyers97], [fn:Meyers01], and [fn:GOF].  scott snyder committed Aug 09, 2017 74 75 76 77 78 79 80 81 82 83 84 85 86 87  This note is not intended to be a fixed set of rigid rules. Rather, it should evolve as experience warrants. * Naming This section contains guidelines on how to name objects in a program. ** Naming of files - *Each class should have one header file, ending with .h'', and one implementation file, ending with .cxx''.* [<>]  88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109  Some exceptions: Small classes used as helpers for another class should generally not go in their own file, but should instead be placed with the larger class. Sometimes several very closely related classes may be grouped together in a single file; in that case, the files should be named after whichever is the primary'' class. A number of related small helper classes (not associated with a particular larger class) may be grouped together in a single file, which should be given a descriptive name. An example of the latter could be a set of classes used as exceptions for a package. For classes in a namespace, the namespace should not be included in the file name. For example, the header for =Trk::Track= should be called =Track.h=. Implementation (.cxx) files that would be empty may be omitted. The use of the .h'' suffix for headers is long-standing ATLAS practice; however, it is unfortunate since language-sensitive editors will then default to using C'' rather than C++'' mode for these files. For emacs, it can help to put a line like this at the start of the file: #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 110 111 112 113 114 115 116 117 118 119 // This file is really -*- C++ -*-. #+END_EXAMPLE ** Meaningful names - *Choose names based on pronounceable English words, common abbreviations, or acronyms widely used in the experiment, except* *for loop iteration variables.* [<>]  120  For example, =nameLength= is better than =nLn=.  scott snyder committed Aug 09, 2017 121   122 123  Use names that are English and self-descriptive. Abbreviations and/or acronyms used should be of common use within the community.  scott snyder committed Aug 09, 2017 124 125 126 127  - *Do not create very similar names.* [<>]  128 129  In particular, avoid names that differ only in case. For example, =track= / =Track=; =c1= / =cl=; =XO= / =X0=.  scott snyder committed Aug 09, 2017 130 131 132 133 134 135 136 137 138  ** Required naming conventions: Generally speaking, you should try to match the conventions used by whatever package you're working on. But please try to always follow these rules: - *Use prefix* =m_= *for private/protected data members of classes.* [<>]  139  Use a lowercase letter after the prefix =m_=.  scott snyder committed Aug 09, 2017 140   141 142  An exception for this is xAOD data classes, where the member names are exposed via ROOT for analysis.  scott snyder committed Aug 09, 2017 143 144 145 146 147 148 149 150  - *Do not start any other names with* =m_=. [<>] - *Do not start names with an underscore. Do not use names that contain anywhere a double underscore.* [<>]  151  Such names are reserved for the use of the compiler and system libraries.  scott snyder committed Aug 09, 2017 152   153 154 155 156 157  The precise rule is that names that contain a double underscore or which start with an underscore followed by an uppercase letter are reserved anywhere, and all other names starting with an underscore are reserved in the global namespace. However, it's good practice to just avoid all names starting with an underscore.  scott snyder committed Aug 09, 2017 158 159 160 161 162 163 164 165 166 167  ** Recommended naming conventions If there is no already-established naming convention for the package you're working on, the following guidelines are recommended as being generally consistent with ATLAS usage. - *Use prefix* =s_= *for private/protected static data members of classes.* [<>]  168  Use a lowercase letter after the prefix =s_=.  scott snyder committed Aug 09, 2017 169 170 171  - *The choice of namespace names should be agreed to by the communities concerned.* [<>]  172 173 174  Don't proliferate namespaces. If the community developing the code has a namespace defined already, use it rather than defining a new one. Examples include =Trk::= for tracking and =InDet::= for inner detector.  scott snyder committed Aug 09, 2017 175 176 177  - *Use namespaces to avoid name conflicts between classes.* [<>]  178 179 180 181 182 183  A name clash occurs when a name is defined in more than one place. For example, two different class libraries could give two different classes the same name. If you try to use many class libraries at the same time, there is a fair chance that you will be unable to compile and link the program because of name clashes. To solve the problem you can use a namespace.  scott snyder committed Aug 09, 2017 184   185 186 187  New code should preferably be put in a namespace, unless typical ATLAS usage is otherwise. For example, ATLAS classes related to the calorimeter conventionally start with Calo'' rather than being in a C++ namespace.  scott snyder committed Aug 09, 2017 188 189 190  - *Start class and enum types with an uppercase letter.* [<>]  191  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 192 193 194 195 196 197 198 class Track; enum State { green, yellow, red }; #+END_EXAMPLE - *Typedef names should start with an uppercase letter if they are public and treated as classes.* [<>]  199  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 200 201 202 203 204 205 typedef vector TrackVector; #+END_EXAMPLE - *Alternatively, a typedef name may start with a lower-case letter and end with* =_t=. [<>]  206 207 208  This form should be reserved for type names which are not treated as classes (e.g., a name for a fundamental type) or names which are private to a class.  scott snyder committed Aug 09, 2017 209   210  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 211 212 213 214 215 216 typedef unsigned int mycounter_t; #+END_EXAMPLE - *Start names of variables, members, and functions with a lowercase letter.* [<>]  217  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 218 219 220 221 double energy; void extrapolate(); #+END_EXAMPLE  222 223  Names starting with =s_= and =m_= should have a lowercase letter following the underscore.  scott snyder committed Aug 09, 2017 224   225 226 227  Exceptions may be made for the case where the name is following standard physics or mathematical notation that would require an uppercase letter; for example, uppercase =E= for energy.  scott snyder committed Aug 09, 2017 228 229 230 231  - *In names that consist of more than one word, write the words together, and start each word that follows the first one with an uppercase letter.* [<>]  232  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 233 234 235 236 237 class OuterTrackerDigit; double depositedEnergy; void findTrack(); #+END_EXAMPLE  238 239 240  Some ATLAS packages also use the convention that names are entirely lowercase and separated by underscores. When modifying an existing package, you should try to be consistent with the existing naming convention.  scott snyder committed Aug 09, 2017 241 242 243 244  - *All package names in the release must be unique, independent of the package's location in the hierarchy.* [<>]  245 246 247  If there is a package, say A/B/C'', already existing, another package may not have the name D/E/C'' because that C'' has already been used. This is required for proper functioning of the build system.  scott snyder committed Aug 09, 2017 248 249 250  - *Underscores should be avoided in package names.* [<>]  251 252 253 254 255 256  The old ATLAS rule was that the =_= should be used in package names when they are composites of one or more acronyms, e.g. =TRT_Tracker=, =AtlasDB_*=. Underscores should be avoided unless they really help with readability and help in avoiding spelling mistakes. =TRTTracker= looks odd because of the double T''. Using underscores in package names will also add to confusion in the multiple-inclusion protection lines.  scott snyder committed Aug 09, 2017 257 258 259  - *Acronyms should be written as all uppercase.* [<>]  260  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 261 262 263 264 METReconstruction, not MetReconstruction MuonCSCValidation, not MuonCscValidation #+END_EXAMPLE  265  Unfortunately, existing code widely uses both forms.  scott snyder committed Aug 09, 2017 266 267 268 269 270 271 272 273 274 275 276 277 278 279 280 281 282 283 284  * Coding This section contains a set of items regarding the content'' of the code. Organization of the code, control flow, object life cycle, conversions, object-oriented programming, error handling, parts of C++ to avoid, portability, are all examples of issues that are covered here. The purpose of the following items is to highlight some useful ways to exploit the features of the programming language, and to identify some common or potential errors to avoid. ** Organizing the code - *Header files must begin and end with multiple-inclusion protection.* [<>]  285  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 286 287 288 289 290 291 #ifndef PACKAGE_CLASS_H #define PACKAGE_CLASS_H // The text of the header goes in here ... #endif // PACKAGE_CLASS_H #+END_EXAMPLE  292 293 294  Header files are often included many times in a program. Because C++ does not allow multiple definitions of a class, it is necessary to prevent the compiler from reading the definitions more than once.  scott snyder committed Aug 09, 2017 295   296 297  The include guard should include both the package name and class name, to ensure that is unique.  scott snyder committed Aug 09, 2017 298   scott snyder committed Sep 17, 2019 299 300 301 302 303 304 305 306 307 308 309 310 311  Don't start the include guard name with an underscore; such names are reserved to the compiler. Be careful to use the same string in the =ifndef= and the =define=. It's useful to get in the habit of using copy/paste here rather than retyping the string. Some compilers support an extension =#pragma once= that has similar functionality. A long time ago, this was sometimes faster, as it allowed the compiler to skip reading headers that have already been read. However, modern compilers will automatically do this optimization based on recognizing header guards. As =#pragma once= is nonstandard and has no compelling advantage, it is best avoided.  scott snyder committed Aug 09, 2017 312   313 314  In some rare cases, a file may be intended to be included multiple times, and thus not have an include guard. Such files should be explicitly  scott snyder committed Sep 17, 2019 315 316  commented as such, and should usually have an extension other than .h''; .def'' is sometimes used for thus purpose.  scott snyder committed Aug 09, 2017 317 318 319  - *Use forward declaration instead of including a header file, if this is sufficient.* [<>]  320 321  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 322 323 324 325 326 327 328 329 330 class Line; class Point { public: // Distance from a line Number distance(const Line& line) const; }; #+END_EXAMPLE  331 332 333  Here it is sufficient to say that =Line= is a class, without giving details which are inside its header. This saves time in compilation and avoids an apparent dependency upon the =Line= header file.  scott snyder committed Aug 09, 2017 334   335 336  Be careful, however: this does not work if =Line= is actually a =typedef= (as is the case, for example, with many of the xAOD classes).  scott snyder committed Aug 09, 2017 337 338 339 340  - *Each header file must contain the declaration for one class only, except for embedded or very tightly coupled classes or collections of small helper classes.* [<>]  341 342 343  This makes your source code files easier to read. This also improves the version control of the files; for example the file containing a stable class declaration can be committed and not changed any more.  scott snyder committed Aug 09, 2017 344   345 346 347 348 349 350 351 352 353  Some exceptions: Small classes used as helpers for another class should generally not go in their own file, but should instead be placed with the larger class. Sometimes several very closely related classes may be grouped together in a single file; in that case, the files should be named after whichever is the primary'' class. A number of related small helper classes (not associated with a particular larger class) may be grouped together in a single file, which should be given a descriptive name. An example of the latter could be a set of classes used as exceptions for a package.  scott snyder committed Aug 09, 2017 354 355 356 357  - *Implementation files must hold the member function definitions for the class(es) declared in the corresponding header file.* [<>]  358  This is for the same reason as for the previous item.  scott snyder committed Aug 09, 2017 359 360 361  - *Ordering of #include statements.* [<>]  362 363 364 365 366 367 368  =#include= directives should generally be listed according to dependency ordering, with the files that have the most dependencies coming first. This implies that the first =#include= in a .cxx'' file should be the corresponding .h'' file, followed by other =#include= directives from the same package. These would then be followed by =#include= directives for other packages, again ordered from most to least dependent. Finally, system =#include= directives should come last.  scott snyder committed Aug 09, 2017 369   370  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 371 372 373 374 375 376 377 378 379 380 381 382 383 384 // Example for CaloCell.cxx // First the corresponding header. #include "CaloEvent/CaloCell.h" // The headers from other ATLAS packages, // from most to least dependent. #include "CaloDetDescr/CaloDetDescrElement.h" #include "SGTools/BaseInfo.h" // Headers from external packages. #include "CLHEP/Geometry/Vector3D.h" #include "CLHEP/Geometry/Point3D.h" // System headers. #include #+END_EXAMPLE  385 386 387  Ordering the =#include= directives in this way gives the best chance of catching problems where headers fail to include other headers that they depend on.  scott snyder committed Aug 09, 2017 388   389 390  Some old guides recommended testing on the C++ header guard around the =#include= directive. This advice is now obsolete and should be avoided.  scott snyder committed Aug 09, 2017 391   392  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 393 394 395 396 397 398 // Obsolete --- don't do this anymore. #ifndef MYPACKAGE_MYHEADER_H # include "MyPackage/MyHeader.h" #endif #+END_EXAMPLE  399 400 401  The rationale for this was to avoid having the preprocessor do redundant reads of the header file. However, current C++ compilers do this optimization on their own, so this serves only to clutter the source.  scott snyder committed Aug 09, 2017 402 403 404  - *Do not use using'' directives or declarations in headers or prior to an #include.* [<>]  405 406  A =using= directive or declaration imports names from one namespace into another, often the global namespace.  scott snyder committed Aug 09, 2017 407   408 409 410 411 412  This does, however, lead to pollution of the global namespace. This can be manageable if it's for a single source file; however, if the directive is in a header file, it can affect many different source files. In most cases, the author of these sources won't be expecting this.  scott snyder committed Aug 09, 2017 413   414  Having =using= in a header can also hide errors. For example:  scott snyder committed Aug 09, 2017 415   416  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 417 418 419 420 421 422 423 424 425 426 427 428 // In first header A.h: using namespace std; // In second header B.h: #include "A.h" // In source file B.cxx #include "B.h" ... vector x; // Missing std! #+END_EXAMPLE  429 430 431 432 433 434 435 436 437 438  Here, a reference to =std::vector= in B.cxx is mistakenly written without the =std::= qualifier. However, it works anyway because of the =using= directive in A.h. But imagine that later B.h is revised so that it no longer uses anything from A.h, so the =#include= of A.h is removed. Suddenly, the reference to =vector= in B.cxx no longer compiles. Now imagine there are several more layers of =#include= and potentially hundreds of affected source files. To try to prevent problems like this, headers should not use =using= outside of classes. (Within a class definition, =using= can have a different meaning that is not covered by this rule.)  scott snyder committed Aug 09, 2017 439   440 441 442 443  For similar reasons, if you have a =using= directive or declaration in a .cxx'' file, it should come after all =#include= directives. Otherwise, the =using= may serve to hide problems with missing namespace qualifications in the headers.  scott snyder committed Aug 09, 2017 444   scott snyder committed Sep 17, 2019 445 446  This rule does not apply when =using= is used to define a new name for a type (similarly to =typedef=).  scott snyder committed Dec 20, 2017 447   scott snyder committed Aug 09, 2017 448 449 450 451 452  ** Control flow - *Do not change a loop variable inside a for loop block.* [<>]  453 454  When you write a for loop, it is highly confusing and error-prone to change the loop variable within the loop body rather than inside the  scott snyder committed Dec 20, 2017 455 456  expression executed after each iteration. It may also inhibit many of the loop optimizations that the compiler can perform.  scott snyder committed Aug 09, 2017 457   scott snyder committed Nov 21, 2017 458 459  - *Prefer range-based for loops.* [<>]  scott snyder committed Sep 17, 2019 460 461  Prefer a range-based for to a loop with explicit iterators. That is, prefer:  scott snyder committed Nov 21, 2017 462 463 464 465 466 467 468 469 470 471 472 473 474 475 476 477 478 479 480 481 482  #+BEGIN_EXAMPLE std::vector v = ...; for (int x : v) { doSomething (x); } #+END_EXAMPLE to #+BEGIN_EXAMPLE std::vector v = ...; for (std::vector::const_iterator it = v.begin(); it != v.end(); ++it) { doSomething (*it); } #+END_EXAMPLE In some cases you can't make this replacement; for example, if you need to call methods on the iterator itself, or you need to manage multiple iterators within the loop. But most simple loops over  scott snyder committed Dec 20, 2017 483  STL ranges are more simply written with a range-based for.  scott snyder committed Nov 21, 2017 484 485   scott snyder committed Sep 17, 2019 486  - *Switch statements should have a default clause.* [<>]  scott snyder committed Aug 09, 2017 487   scott snyder committed Sep 17, 2019 488 489 490  A =switch= statement should have a =default= clause, rather than just falling off the bottom, as a cue to the reader that this case was expected.  scott snyder committed Aug 09, 2017 491   scott snyder committed Sep 17, 2019 492 493 494 495 496 497  In some cases, a =switch= statement may be on a =enum= and includes =case= clauses for all possible values of the =enum=. In such cases, a =default= cause is not required. Recent compilers will generate warnings if some elements of an =enum= are not handled in a =switch=. This mitigates the risk that a =switch= does not get updated after a new =enum= value is added.  scott snyder committed Aug 09, 2017 498 499 500 501  - *Each clause of a switch statement must end with* =break=. [<>]  502 503  If you must fall through'' from one switch clause to another (excluding the trivial case of a clause with no statements),  scott snyder committed Sep 17, 2019 504 505  this should be explicitly indicated using the =fallthough= attribute. This should, however, be a rare case.  scott snyder committed Aug 09, 2017 506   scott snyder committed Nov 16, 2017 507 508 509 510  #+BEGIN_EXAMPLE switch (case) { case 1: doSomething();  scott snyder committed Sep 17, 2019 511  [[fallthrough]];  scott snyder committed Nov 16, 2017 512 513 514 515 516 517 case 2: doSomethingMore(); break; ... #+END_EXAMPLE  scott snyder committed Sep 17, 2019 518 519 520  Recent compilers will warn about such constructs unless you use the attribute or a special comment. For new code, using the attribute is preferred.  scott snyder committed Nov 16, 2017 521   scott snyder committed Aug 09, 2017 522 523 524 525  - *An if-statement which does not fit in one line must have braces around the conditional statement.* [<>]  526 527  This makes code much more readable and reliable, by clearly showing the flow paths.  scott snyder committed Aug 09, 2017 528   529 530 531  The addition of a final else is particularly important in the case where you have if/else-if. To be safe, even single statements should be explicitly blocked by ={}=.  scott snyder committed Aug 09, 2017 532   533  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 534 535 536 537 538 539 540 541 542 543 544 545 546 547 if (val == thresholdMin) { statement; } else if (val == thresholdMax) { statement; } else { statement; // handles all other (unforeseen) cases } #+END_EXAMPLE - *Do not use goto.* [<>]  548  Use =break= or =continue= instead.  scott snyder committed Aug 09, 2017 549   550 551 552  This statement remains valid also in the case of nested loops, where the use of control variables can easily allow to break the loop, without using =goto=.  scott snyder committed Aug 09, 2017 553   554 555  =goto= statements decrease readability and maintainability and make testing difficult by increasing the complexity of the code.  scott snyder committed Aug 09, 2017 556   557 558 559  If =goto= statements must be used, it's better to use them for forward branching than backwards, and the functions involved should be kept short.  scott snyder committed Aug 09, 2017 560 561 562 563 564 565 566 567  ** Object life cycle *** Initialization of variables and constants - *Declare each variable with the smallest possible scope and initialize it at the same time.* [<>]  568 569 570  It is best to declare variables close to where they are used. Otherwise you may have trouble finding out the type of a particular variable.  scott snyder committed Aug 09, 2017 571   572 573  It is also very important to initialize the variable immediately, so that its value is well defined.  scott snyder committed Aug 09, 2017 574   575  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 576 577 578 579 580 581 582 int value = -1; // initial value clearly defined int maxValue; // initial value undefined, NOT recommended #+END_EXAMPLE - *Avoid use of magic literals'' in the code.* [<>]  583 584 585  If some number or string has a particular meaning, it's best to declare a symbol for it, rather than using it directly. This is especially true if the same value must be used consistently in multiple places.  scott snyder committed Aug 09, 2017 586   587  Bad example:  scott snyder committed Aug 09, 2017 588   589  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 590 591 592 593 594 595 596 597 598 599 600 601 602 603 604 class A { ... TH1* m_array[10]; }; void A::foo() { for (int i = 0; i < 10; i++) { m_array[i] = dynamic_cast (gDirectory()->Get (TString ("hist_") + TString::Itoa(i,10))); } #+END_EXAMPLE  605  Better example:  scott snyder committed Aug 09, 2017 606   607  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 608 609 610 611 612 613 614 615 616 617 618 619 620 621 622 623 624 625 626 627 class A { ... static const s_numberOfHistograms = 10; static TString s_histPrefix; TH1* m_array[s_numberOfHistograms]; }; TString s_histPrefix = "hist_"; void A::foo() { for (int i = 0; i < s_numberOfHistograms; i++) { TString istr = TString::Itoa (i, 10); // base 10 m_array[i] = dynamic_cast (gDirectory()->Get (s_histPrefix + istr); } #+END_EXAMPLE  628 629 630 631 632 633 634 635 636  It is not necessary to turn /every/ literal into a symbol. For example, the 10' in the example above in the =Itoa= call, which gives the base for the conversion, would probably not benefit from being made a symbol, though a comment might be helpful. Similarly, sometimes =reserve()= is called on a =std::vector= before it is filled with a value that is essentially arbitrary. It probably also doesn't help to make this a symbol, but again, a comment would be helpful. Strings containing text to be written as part of a log message are also best written literally.  scott snyder committed Aug 09, 2017 637   638 639 640  In general, though, if you write a literal value other than 0', 1', =true=, =false=, or a string used in a log message, you should consider defining a symbol for it.  scott snyder committed Aug 09, 2017 641 642 643 644 645  - *Declare each type of variable in a separate declaration statement, and do not declare different types (e.g. int and int pointer) in one declaration statement.* [<>]  646 647  Declaring multiple variables on the same line is not recommended. The code will be difficult to read and understand.  scott snyder committed Aug 09, 2017 648   649 650 651  Some common mistakes are also avoided. Remember that when you declare a pointer, a unary pointer is bound only to the variable that immediately follows.  scott snyder committed Aug 09, 2017 652   653  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 654 655 656 657 658 659 660 int i, *ip, ia[100], (*ifp)(); // Not recommended // recommended way: LoadModule* oldLm = 0; // pointer to the old object LoadModule* newLm = 0; // pointer to the new object #+END_EXAMPLE  661 662  Bad example: both =ip= and =jp= were intended to be pointers to integers, but only =ip= is --- =jp= is just an integer!  scott snyder committed Aug 09, 2017 663   664  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 665 666 667 668 669 670 671 int* ip, jp; #+END_EXAMPLE - *Do not use the same variable name in outer and inner scope.* [<>]  672 673 674 675 676  Otherwise the code would be very hard to understand; and it would certainly be very error prone. Some compilers will warn about this.  scott snyder committed Aug 09, 2017 677   678 679  - *Be conservative in using* =auto=. [<>]  scott snyder committed Sep 17, 2019 680  The =auto= keyword allows one to omit explicitly  681 682 683 684 685 686 687  writing types that the compile can deduce. Examples: #+BEGIN_EXAMPLE auto x = 10; // Type int deduced auto y = 42ul; // Type unsigned long deduced. auto it = vec.begin(); // Iterator type deduced #+END_EXAMPLE  scott snyder committed Aug 09, 2017 688   689 690 691 692 693 694 695 696 697 698 699 700 701 702 703 704 705 706 707 708 709 710 711  Some authorities have recommended using =auto= pretty much everywhere you can (calling it auto almost always''). However, our experience has been that this adversely affects the readability and robustness of the code. It generally helps a reader to understand what the code is doing if the type is apparent, but with =auto=, it often isn't. Using =auto= also makes it more difficult to find places where a particular type is used when searching the code with tools like lxr. It can also make it more difficult to track errors back to their source: #+BEGIN_EXAMPLE const Foo* doSomething(); ... a lot of code here ... auto foo = doSomething(); // What is the type of foo here? You have to look up // doSomething() in order to find out! Makes it much // harder to find all places where the type Foo gets used. // If the return type of doSomething() changes, you'll get // an error here, not at the doSomething() call. foo->doSomethingElse(); #+END_EXAMPLE  scott snyder committed Sep 17, 2019 712 713 714 715 716 717 718 719 720 721 722 723 724 725 726 727 728 729 730 731 732 733  =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 > 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 > 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.  734 735 736 737 738 739 740 741 742 743 744 745 746 747 748 749 750 751 752 753 754 755 756 757 758 759 760 761 762 763 764 765 766 767 768 769 770 771 772 773 774 775 776 777 778 779 780  The current recommendation is to generally not use =auto= in place of a (possibly-qualified) simple type: #+BEGIN_EXAMPLE // Use these int x = 42; const Foo* foo = doSomething(); for (const CaloCell* cell : caloCellContainer) ... Foo foo (x); // Rather than these auto x = 42; auto foo = doSomething(); for (auto cell : caloCellContainer) ... auto foo = Foo {x}; #+END_EXAMPLE There are three sorts of places where it generally makes sense to use =auto=. - When the type is already evident in the expression and the declaration would be redundant. This is usually the case for expressions with =new= or =make_unique=. #+BEGIN_EXAMPLE // auto is fine here. auto foo = new Foo; auto ufoo = std::make_unique(); #+END_EXAMPLE - When you need a declaration for a complicated derived type, where the type itself isn't of much interest. #+BEGIN_EXAMPLE // Fine to use auto here; the full name of the type // is too cumbersome to be useful. std::map m = ..; auto ret = m.insert (std::make_pair (1, "x")); if (ret.second) .... #+END_EXAMPLE - =auto= may also be useful in writing generic template code. In general, the decision as to whether or not to use =auto= should be made on the basis of what makes the code easier to _read_. It is bad practice to use it simply to save a few characters of typing.  scott snyder committed Aug 09, 2017 781 782 783 784 785 786 787  *** Constructor initializer lists - *Let the order in the initializer list be the same as the order of the declarations in the header file: first base classes, then data members.* [<>]  788 789  It is legal in C++ to list initializers in any order you wish, but you should list them in the same order as they will be called.  scott snyder committed Aug 09, 2017 790   791 792 793 794  The order in the initializer list is irrelevant to the execution order of the initializers. Putting initializers for data members and base classes in any order other than their actual initialization order is therefore highly confusing and can lead to errors.  scott snyder committed Aug 09, 2017 795   796 797 798 799 800 801  Class members are initialized in the order of their declaration in the class; the order in which they are listed in a member initialization list makes no difference whatsoever! So if you hope to understand what is really going on when your objects are being initialized, list the members in the initialization list in the order in which those members are declared in the class.  scott snyder committed Aug 09, 2017 802   803 804 805  Here, in the bad example, =m_data= is initialized first (as it appears in the class) /before/ =m_size=, even though =m_size= is listed first. Thus, the =m_data= initialization will read uninitialized data from =m_size=.  scott snyder committed Aug 09, 2017 806   807  Bad example:  scott snyder committed Aug 09, 2017 808   809  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 810 811 812 813 814 815 816 817 818 819 820 821 822 823 824 825 826 827 class Array { public: Array(int lower, int upper); private: int* m_data; unsigned m_size; int m_lowerBound; int m_upperBound; }; Array::Array(int lower, int upper) : m_size(upper-lower+1), m_lowerBound(lower), m_upperBound(upper), m_data(new int[m_size]) { ... #+END_EXAMPLE  828  Correct example:  scott snyder committed Aug 09, 2017 829   830  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 831 832 833 834 835 836 837 838 839 840 841 842 843 844 845 846 847 class Array { public: Array(int lower, int upper); private: unsigned m_size; int m_lowerBound; int m_upperBound; int* m_data; }; Array::Array(int lower, int upper) : m_size(upper-lower+1), m_lowerBound(lower), m_upperBound(upper), m_data(new int[m_size]) { ... #+END_EXAMPLE  848 849 850  Virtual base classes are always initialized first, then base classes, data members, and finally the constructor body for the derived class is run.  scott snyder committed Aug 09, 2017 851   852  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 853 854 855 856 857 858 859 860 861 862 863 864 865 866 867 868 869 870 871 872 873 874 875 876 877 878 879 880 881 class Derived : public Base // Base is number 1 { public: explicit Derived(int i); // The keyword explicit prevents the constructor // from being called implicitly. // int x = 1; // Derived dNew = x; // will not work Derived(); private: int m_jM; // m_jM is number 2 Base m_bM; // m_bM is number 3 }; Derived::Derived(int i) : Base(i), m_jM(i), m_bM(i) { // Recommended order 1 2 3 ... } #+END_EXAMPLE *** Copying of objects - *A function must never return, or in any other way give access to, references or pointers to local variables outside the scope in which they are declared.* [<>]  882 883 884  Returning a pointer or reference to a local variable is always wrong because it gives the user a pointer or reference to an object that no longer exists.  scott snyder committed Aug 09, 2017 885   886  Bad example:  scott snyder committed Aug 09, 2017 887   888 889  You are using a complex number class, =Complex=, and you write a method that looks like this:  scott snyder committed Aug 09, 2017 890   891 892 893  #+BEGIN_EXAMPLE Complex& calculateC1 (const Complex& n1, const Complex& n2)  scott snyder committed Aug 09, 2017 894 895 896 897 898 899 900 901 { double a = n1.getReal()-2*n2.getReal(); double b = n1.getImaginary()*n2.getImaginary(); // create local object Complex C1(a,b); // return reference to local object  902  // the object is destroyed on exit from this function:  scott snyder committed Aug 09, 2017 903 904 905 906 907  // trouble ahead! return C1; } #+END_EXAMPLE  908  In fact, most compilers will spot this and issue a warning.  scott snyder committed Aug 09, 2017 909   910 911  This particular function would be better written to return the result by value:  scott snyder committed Aug 09, 2017 912   913  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 914 915 916 917 918 919 920 921 922 923 924 925 926 Complex calculateC1 (const Complex& n1, const Complex& n2) { double a = n1.getReal()-2*n2.getReal(); double b = n1.getImaginary()*n2.getImaginary(); return Complex(a,b); } #+END_EXAMPLE - *If objects of a class should never be copied, then the copy constructor and the copy assignment operator should be deleted.* [<>]  927 928 929  Ideally the question whether the class has a reasonable copy semantic will naturally be a result of the design process. Do not define a copy method for a class that should not have it.  scott snyder committed Aug 09, 2017 930   931 932  By deleting the copy constructor and copy assignment operator, you can make a class non-copyable.  scott snyder committed Aug 09, 2017 933   934  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 935 936 937 938 939 940 941 942 943 // There is only one ATLASExperimentalHall, // and that should not be copied class ATLASExperimentalHall { public: ATLASExperimentalHall(); ~ATLASExperimentalHall(); // Delete copy constructor --- disallow copying.  944 945  ATLASExperimentalHall(const ATLASExperimentalHall& ) = delete;  scott snyder committed Aug 09, 2017 946 947  // Delete assignment operator --- disallow assignment.  948 949  ATLASExperimentalHall& operator=(const ATLAS_ExperimentalHall&) = delete;  scott snyder committed Aug 09, 2017 950 951 952 }; #+END_EXAMPLE  scott snyder committed Sep 17, 2019 953  In older versions of the language, this was achieved  scott snyder committed Nov 16, 2017 954  by declaring the deleted methods as private (and not implementing them).  scott snyder committed Sep 17, 2019 955  For new code, prefer explicitly deleting the functions.  scott snyder committed Aug 09, 2017 956   957  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 958 959 960 961 962 963 964 965 966 967 968 969 970 971 972 973 974 975 976 // There is only one ATLASExperimentalHall, // and that should not be copied class ATLASExperimentalHall { public: ATLASExperimentalHall(); ~ATLASExperimentalHall(); private: // Disallow copy constructor and assignment. ATLASExperimentalHall(const ATLASExperimentalHall& ); ATLASExperimentalHall& operator= (const ATLAS_ExperimentalHall&); }; #+END_EXAMPLE - *If a class owns memory via a pointer data member, then the copy constructor, the assignment operator, and the destructor should all be implemented.* [<>]  977 978 979 980 981 982 983 984 985 986 987 988 989 990 991  The compiler will generate a copy constructor, an assignment operator, and a destructor if these member functions have not been declared. A compiler-generated copy constructor does memberwise initialization and a compiler-generated copy assignment operator does memberwise assignment of data members and base classes. For classes that manage resources (examples: memory (new), files, sockets) the generated member functions probably have the wrong behavior and must be implemented by the developer. You have to decide if the resources pointed to must be copied as well (deep copy), and implement the correct behavior in the operators. Of course, the constructor and destructor must be implemented as well. Bad Example: #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 992 993 994 995 996 997 998 999 1000 1001 1002 class String { public: String(const char *value=0); ~String(); // destructor but no copy constructor // or assignment operator private: char *m_data; }; String::String(const char *value)  1003 1004 1005 { // correct behavior implemented in constructor m_data = new char[strlen(value)]; // fill m_data }  scott snyder committed Aug 09, 2017 1006 String::~String()  1007 1008 1009 { // correct behavior implemented in destructor delete m_data; }  scott snyder committed Aug 09, 2017 1010 1011 1012  ...  1013 // declare and construct a ==> m_data points to "Hello"  scott snyder committed Aug 09, 2017 1014 1015 1016 String a("Hello"); // open new scope  1017 { // declare and construct b ==> m_data points to "World"  scott snyder committed Aug 09, 2017 1018 1019 1020 1021  String b("World"); b=a; // execute default op= as synthesized by compiler ==>  1022 1023  // memberwise assignment i.e. for pointers (m_data) // bitwise copy  scott snyder committed Aug 09, 2017 1024 1025 1026 1027 1028 1029 1030 1031 1032  // ==> m_data of "a" and "b" now point to the same string // "Hello" // ==> 1) memory b used to point to never deleted ==> // possible memory leak // 2) when either a or b goes out of scope, // its destructor will delete the memory // still pointed to by the other }  1033 1034 // close scope: b''s destructor called; // memory still pointed to by a' deleted!  scott snyder committed Aug 09, 2017 1035 1036 1037 1038 1039 1040 1041 String c=a; // but m_data of "a" is undefined!! #+END_EXAMPLE - *Assignment member functions must work correctly when the left and right operands are the same object.* [<>]  1042 1043 1044  This requires some care when writing assignment code, as the case (when left and right operands are the same) may require that most of the code is bypassed.  scott snyder committed Aug 09, 2017 1045   1046  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 1047 1048 1049 1050 1051 1052 1053 1054 1055 1056 1057 1058 1059 A& A::operator=(const A& a) { if (this != &a) { // beware of s=s - "this" and "a" are the same object // ... implementation of operator= } } #+END_EXAMPLE ** Conversions - *Use explicit rather than implicit type conversion.* [<>]  1060 1061 1062 1063  Most conversions are bad in some way. They can make the code less portable, less robust, and less readable. It is therefore important to use only explicit conversions. Implicit conversions are almost always bad.  scott snyder committed Aug 09, 2017 1064   1065 1066  In general, casts should be strongly discouraged, especially the old style C casts.  scott snyder committed Aug 09, 2017 1067 1068 1069 1070 1071  - *Use the C++ cast operators* (=dynamic_cast= *and* =static_cast=) *instead of the C-style casts.* [<>]  1072 1073 1074  The new cast operators give the user a way to distinguish between different types of casts, and to ensure that casts only do what is intended and nothing else.  scott snyder committed Aug 09, 2017 1075   1076 1077 1078 1079 1080  The C++ =static_cast= operator allows explicitly requesting allowed implicit conversions and between integers and enums. It also allows casting pointers up and down a class hierarchy (as long as there's no virtual inheritance), but no checking is done when casting from a less- to a more-derived type.  scott snyder committed Aug 09, 2017 1081   1082 1083 1084 1085 1086  The C++ =dynamic_cast= operator is used to perform safe casts down or across an inheritance hierarchy. One can actually determine whether the cast succeeded because failed casts are indicated either by a =bad_cast= exception or a null pointer. The use of this type of information at run time is called Run-Time Type Identification (RTTI).  scott snyder committed Aug 09, 2017 1087   1088  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 1089 1090 1091 1092 1093 1094 1095 1096 1097 1098 1099 1100 1101 1102 1103 1104 1105 int n = 3; double r = static_cast(n) * a; class Base { }; class Derived : Base { }; void f(Derived* d_ptr) { // if the following cast is inappropriate // a null pointer will be returned! Base* b_ptr = dynamic_cast(d_ptr); // ... } #+END_EXAMPLE - *Do not convert const objects to non-const.* [<>]  1106  In general you should never cast away the constness of objects.  scott snyder committed Aug 09, 2017 1107   1108 1109 1110 1111  If you have to use a =const_cast= to remove =const=, either you're writing some low-level code that that's deliberately subverting the C++ type system, or you have some problem in your design or implementation that the =const_cast= is papering over.  scott snyder committed Aug 09, 2017 1112   1113 1114 1115 1116  Sometimes you're forced to use a =const_cast= due to problems with external libraries. But if the library in question is maintained by ATLAS, then try to get it fixed in the original library before resorting to =const_cast=.  scott snyder committed Aug 09, 2017 1117   1118 1119 1120 1121 1122 1123  The keyword =mutable= allows data members of an object that have been declared const to remain modifiable, thus reducing the need to cast away constness. The =mutable= keyword should only be used for variables which are used for caching information. In other words, the object appears not to have changed but it has stored something to save time on subsequent use.  scott snyder committed Aug 09, 2017 1124 1125 1126  - *Do not use* =reinterpret_cast=. [<>]  1127 1128 1129 1130 1131 1132 1133 1134 1135 1136 1137 1138 1139 1140 1141 1142 1143 1144 1145 1146 1147 1148 1149 1150 1151 1152 1153 1154 1155 1156 1157 1158  =reinterpret_cast= is machine-, compiler- and compile-options-dependent. It is a way of forcing a compiler to accept a type conversion which is dependent on implementation. It blows away type-safety, violates encapsulation and more importantly, can lead to unpredictable results. =reinterpret_cast= has legitimate uses, such as low-level code which deliberately goes around the C++ type system. Such code should usually be found only in the core and framework packages. Exception: =reinterpret_cast= is required in some cases if one is not using old-style casts. It is required for example if you wish to convert a callback function signature (X11, expat, Unix signal handlers are common causes). Some external libraries (X11 in particular) depend on casting function pointers. If you absolutely have to work around limitations in external libraries, you may of course use it. One particularly nasty case to be aware of and to avoid is /pointer aliasing/. If two pointers have different types, the compiler may assume that they cannot point at the same object. For example, in this function: #+BEGIN_EXAMPLE int convertAndBuffer (int* buf, float x) { float* fbuf = reinterpret_cast(buf); *fbuf = x; return *buf; } #+END_EXAMPLE the compiler is entitled to rewrite it as  scott snyder committed Aug 09, 2017 1159   1160 1161 1162 1163 1164 1165 1166 1167 1168 1169 1170  #+BEGIN_EXAMPLE int convertAndBuffer (int* buf, float x) { int ret = *buf; float* fbuf = reinterpret_cast(buf); *fbuf = x; return ret; } #+END_EXAMPLE (As a special case, you can safely convert any pointer type to or from  scott snyder committed Nov 21, 2017 1171  a =char*=.) The proper way to do such a conversion is with a union.  scott snyder committed Aug 09, 2017 1172 1173 1174 1175 1176 1177 1178 1179 1180 1181  ** The class interface *** Inline functions - *Header files must contain no implementation except for small functions to be inlined. These inlines must appear at the end of the header after* *the class definition.* [<>]  1182 1183 1184  If you have many inline functions, it is usually better to split them out into a separate file, with extension .icc'', that is included at the end of the header.  scott snyder committed Aug 09, 2017 1185   1186 1187 1188 1189 1190 1191  Inline functions can improve the performance of your program; but they also can increase the overall size of the program and thus, in some cases, have the opposite result. It can be hard to know exactly when inlining is appropriate. As a rule of thumb, inline only very simple functions to start with (one or two lines). You can use profiling information to identify other functions that would benefit from inlining.  scott snyder committed Aug 09, 2017 1192   1193 1194 1195  Use of inlining makes debugging hard and, even worse, can force a complete release rebuild or large scale recompilation if the inline definition needs to be changed.  scott snyder committed Aug 09, 2017 1196 1197 1198 1199 1200  *** Argument passing and return values  1201 1202  - *Pass an unmodifiable argument by value only if it is of built-in type or small; otherwise, pass the argument by const reference (or by const pointer if it may be null).* [<>]  scott snyder committed Aug 09, 2017 1203   1204 1205 1206 1207 1208 1209  An object is considered small if it is a built-in type or if it contains at most one small object. Built-in types such as =char=, =int=, and =double= can be passed by value because it is cheap to copy such variables. If an object is larger than the size of its reference (typically 64 bits), it is not efficient to pass it by value. Of course, a built-in type can be passed by reference when appropriate.  scott snyder committed Aug 09, 2017 1210   1211  #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 1212 1213 1214 1215 1216 void func(char c); // OK void func(int i); // OK void func(double d); // OK void func(complex c); // OK  1217 void func(Track t); // not good, since Track is large, so  scott snyder committed Aug 09, 2017 1218 1219 1220  // there is an overhead in copying t. #+END_EXAMPLE  1221 1222 1223 1224  Arguments of class type are often costly to copy, so it is preferable to pass a =const= reference to such objects; in this way the argument is not copied. Const access guarantees that the function will not change the argument.  scott snyder committed Aug 09, 2017 1225   1226 1227 1228 1229 1230  In terms of efficiency, passing by pointer is the same as passing by reference. However, passing by reference is preferred, unless it is possible to the object to be missing from the call. #+BEGIN_EXAMPLE  scott snyder committed Aug 09, 2017 1231 1232 1233 void func(const LongString& s); // const reference #+END_EXAMPLE  1234 1235 1236 1237 1238 1239 1240 1241 1242 1243 1244 1245 1246 1247 1248 1249 1250 1251 1252 1253 1254 1255 1256 1257 1258 ` - *If an argument may be modified, pass it by non-const reference and clearly document the intent.* [<>] For example: #+BEGIN_EXAMPLE // Track @c t is updated by the addition of hit @c h. void updateTrack(const Hit& h, Track& t); #+END_EXAMPLE Again, passing by references is preferred, but a pointer may be used if the object can be null. - *Use* =unique_ptr= *to pass ownership of an object to a function.* [<>] To pass ownership of an object into a function, use =unique_ptr= (by value): #+BEGIN_EXAMPLE void foo (std::unique_ptr