rules.org 110 KB
Newer Older
1
2
#+MACRO: version 0.7
#+LaTeX_HEADER: \def\rulesversion{0.7}
3
#+TITLE: ATLAS C++ coding guidelines, version {{{version}}}
4
#+AUTHOR: Scott Snyder (BNL), Shaun Roe (CERN), and the former ATLAS Quality Control group
5
#+EMAIL: Correspondence to snyder@bnl.gov.
6
# Checked with org-mode 9.1.9 (as bundled with emacs 26.2)
7
# To export to PDF use `org-latex-export-to-pdf'.
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's avatar
scott snyder committed
10

11
# Along with the change to org-export-dictionary below, this changes
scott snyder's avatar
scott snyder committed
12
13
14
# the `Footnotes' section in HTML to `References'
#+LANGUAGE: en-references

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's avatar
scott snyder committed
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}
39
40
41
#+LaTeX_HEADER: \rfoot{Version \rulesversion}

#+LaTeX_HEADER: \def\mylabel#1{\label{#1}#1}
scott snyder's avatar
scott snyder committed
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,
55
look up references on ``literate programming,'' such as [fn:Knuth84].)
scott snyder's avatar
scott snyder committed
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,
64
[[https://cds.cern.ch/record/685315][ATL-SOFT-2002-001]] [fn:Atlas02], which was last revised in 2003.  This itself 
scott snyder's avatar
scott snyder committed
65
derived from work done by the CERN ``Project support team''
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's avatar
scott snyder committed
67
These previous guidelines have been significantly revised
68
to take into account the evolution of the C++ language [fn:Cxx17],
scott snyder's avatar
scott snyder committed
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
73
found in [fn:Meyers97], [fn:Meyers01], and [fn:GOF].
scott snyder's avatar
scott snyder committed
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''.*  [<<source-naming>>]

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's avatar
scott snyder committed
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.*  [<<meaningful-names>>]

120
   For example, =nameLength= is better than =nLn=.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
124
125
126
127


 - *Do not create very similar names.*  [<<no-similar-names>>]

128
129
   In particular, avoid names that differ only in case.  For example,
   =track= / =Track=; =c1= / =cl=; =XO= / =X0=.
scott snyder's avatar
scott snyder committed
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.*  [<<data-member-naming>>]

139
   Use a lowercase letter after the prefix =m_=.
scott snyder's avatar
scott snyder committed
140

141
142
   An exception for this is xAOD data classes, where the member names are
   exposed via ROOT for analysis.
scott snyder's avatar
scott snyder committed
143
144
145
146
147
148
149
150


 - *Do not start any other names with* =m_=. [<<m-prefix-reserved>>]


 - *Do not start names with an underscore.  Do not use names that contain
    anywhere a double underscore.* [<<system-reserved-names>>]

151
   Such names are reserved for the use of the compiler and system libraries.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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.* [<<static-members>>]

168
   Use a lowercase letter after the prefix =s_=.
scott snyder's avatar
scott snyder committed
169
170
171

 - *The choice of namespace names should be agreed to by the communities concerned.*  [<<namespace-naming>>]

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's avatar
scott snyder committed
175
176
177

 - *Use namespaces to avoid name conflicts between classes.*  [<<use-namespaces>>]

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's avatar
scott snyder committed
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's avatar
scott snyder committed
188
189
190

 - *Start class and enum types with an uppercase letter.*  [<<class-naming>>]

191
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<typedef-naming>>]

199
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
200
201
202
203
204
205
typedef vector<MCParticleKinematics*> TrackVector;
#+END_EXAMPLE

 - *Alternatively, a typedef name may start with a lower-case letter
   and end with* =_t=.  [<<typedef-naming-2>>]

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's avatar
scott snyder committed
209

210
  #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
211
212
213
214
215
216
typedef unsigned int mycounter_t;
#+END_EXAMPLE


 - *Start names of variables, members, and functions with a lowercase letter.*  [<<variable-and-function-naming>>]

217
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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.*  [<<compound-names>>]

232
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
241
242
243
244

 - *All package names in the release must be unique, independent of the
   package's location in the hierarchy.*  [<<unique-package-names>>]

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's avatar
scott snyder committed
248
249
250

 - *Underscores should be avoided in package names.*  [<<no-underscores-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's avatar
scott snyder committed
257
258
259

 - *Acronyms should be written as all uppercase.*  [<<uppercase-acronyms>>]

260
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
261
262
263
264
METReconstruction, not MetReconstruction
MuonCSCValidation, not MuonCscValidation
#+END_EXAMPLE

265
   Unfortunately, existing code widely uses both forms.
scott snyder's avatar
scott snyder committed
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.* [<<header-guards>>]

285
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
295

296
297
   The include guard should include both the package name and class name,
   to ensure that is unique.
scott snyder's avatar
scott snyder committed
298

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's avatar
scott snyder committed
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
315
316
   commented as such, and should usually have an extension other than ``.h'';
   ``.def'' is sometimes used for thus purpose.
scott snyder's avatar
scott snyder committed
317
318
319

 - *Use forward declaration instead of including a header file,
    if this is sufficient.*  [<<forward-declarations>>]
320
321
   
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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.*  [<<one-class-per-source>>]

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's avatar
scott snyder committed
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's avatar
scott snyder committed
354
355
356
357

 - *Implementation files must hold the member function definitions for
   the class(es) declared in the corresponding header file.* [<<implementation-file>>]

358
   This is for the same reason as for the previous item.
scott snyder's avatar
scott snyder committed
359
360
361

 - *Ordering of #include statements.*  [<<include-ordering>>]

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's avatar
scott snyder committed
369

370
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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 <cmath>
#+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's avatar
scott snyder committed
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's avatar
scott snyder committed
391

392
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
402
403
404

 - *Do not use ``using'' directives or declarations in headers or prior to an #include.* [<<no-using-in-headers>>]

405
406
   A =using= directive or declaration imports names from one namespace 
   into another, often the global namespace.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
413

414
   Having =using= in a header can also hide errors.  For example:
scott snyder's avatar
scott snyder committed
415

416
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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<int> 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's avatar
scott snyder committed
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's avatar
scott snyder committed
444

445
446
   This rule does not apply when =using= is used to define a new name
   for a type (similarly to =typedef=).
447

scott snyder's avatar
scott snyder committed
448
449
450
451
452

** Control flow

 - *Do not change a loop variable inside a for loop block.*  [<<do-not-modify-for-variable>>]

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
455
456
   expression executed after each iteration.  It may also inhibit many
   of the loop optimizations that the compiler can perform.
scott snyder's avatar
scott snyder committed
457

458
459
 - *Prefer range-based for loops.*  [<<prefer-range-based-for>>]

460
461
   Prefer a range-based for to a loop
   with explicit iterators.  That is, prefer:
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<int> v = ...;
for (int x : v) {
  doSomething (x);
}
#+END_EXAMPLE

   to
   #+BEGIN_EXAMPLE
std::vector<int> v = ...;
for (std::vector<int>::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
483
   STL ranges are more simply written with a range-based for.
484
485


486
 - *Switch statements should have a default clause.*  [<<switch-default>>]
scott snyder's avatar
scott snyder committed
487

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's avatar
scott snyder committed
491

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's avatar
scott snyder committed
498
499
500
501


 - *Each clause of a switch statement must end with* =break=.  [<<switch-break>>]

502
503
   If you must ``fall through'' from one switch clause to another 
   (excluding the trivial case of a clause with no statements),
504
505
   this should be explicitly indicated using the =fallthough= attribute.
   This should, however, be a rare case.
scott snyder's avatar
scott snyder committed
506

scott snyder's avatar
scott snyder committed
507
508
509
510
   #+BEGIN_EXAMPLE
switch (case) {
case 1:
  doSomething();
511
  [[fallthrough]];
scott snyder's avatar
scott snyder committed
512
513
514
515
516
517
case 2:
  doSomethingMore();
  break;
...
#+END_EXAMPLE

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's avatar
scott snyder committed
521

scott snyder's avatar
scott snyder committed
522
523
524
525

 - *An if-statement which does not fit in one line must have braces
   around the conditional statement.*  [<<if-bracing>>]

526
527
   This makes code much more readable and reliable, by clearly showing
   the flow paths.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
532

533
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<no-goto>>]

548
   Use =break= or =continue= instead.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
553

554
555
   =goto= statements decrease readability and maintainability and make
   testing difficult by increasing the complexity of the code.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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.*  [<<variable-initialization>>]

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's avatar
scott snyder committed
571

572
573
    It is also very important to initialize the variable immediately, so
    that its value is well defined.
scott snyder's avatar
scott snyder committed
574

575
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<no-magic-literals>>]

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's avatar
scott snyder committed
586

587
    Bad example:
scott snyder's avatar
scott snyder committed
588

589
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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<TH1*>
      (gDirectory()->Get (TString ("hist_") +
       TString::Itoa(i,10)));
}
#+END_EXAMPLE

605
    Better example:
scott snyder's avatar
scott snyder committed
606

607
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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<TH1*>
     (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's avatar
scott snyder committed
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's avatar
scott snyder committed
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.*  [<<separate-declarations>>]

646
647
    Declaring multiple variables on the same line is not recommended. The
    code will be difficult to read and understand.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
652

653
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
663

664
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
665
666
667
668
669
670
671
int* ip, jp;
#+END_EXAMPLE



 - *Do not use the same variable name in outer and inner scope.*  [<<no-variable-shadowing>>]

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's avatar
scott snyder committed
677

678
679
 - *Be conservative in using* =auto=.  [<<using-auto>>]

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's avatar
scott snyder committed
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

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<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.

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<Foo>();
#+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<int, std::string> 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's avatar
scott snyder committed
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.*  [<<member-initializer-ordering>>]

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's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
806

807
    Bad example:
scott snyder's avatar
scott snyder committed
808

809
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
829

830
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
851

852
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<no-refs-to-locals>>]

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's avatar
scott snyder committed
885

886
    Bad example:
scott snyder's avatar
scott snyder committed
887

888
889
    You are using a complex number class, =Complex=, and you write a method
    that looks like this:
scott snyder's avatar
scott snyder committed
890

891
892
893
    #+BEGIN_EXAMPLE
Complex&
calculateC1 (const Complex& n1, const Complex& n2)
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
909

910
911
    This particular function would be better written to return the result
    by value:
scott snyder's avatar
scott snyder committed
912

913
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<copy-protection>>]

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's avatar
scott snyder committed
930

931
932
    By deleting the copy constructor and copy assignment operator, you can make a
    class non-copyable.
scott snyder's avatar
scott snyder committed
933

934
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
946
947

  // Delete assignment operator --- disallow assignment.
948
949
  ATLASExperimentalHall&
  operator=(const ATLAS_ExperimentalHall&) = delete;
scott snyder's avatar
scott snyder committed
950
951
952
};
#+END_EXAMPLE

953
    In older versions of the language, this was achieved
scott snyder's avatar
scott snyder committed
954
    by declaring the deleted methods as private (and not implementing them).
955
    For new code, prefer explicitly deleting the functions.
scott snyder's avatar
scott snyder committed
956

957
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<define-copy-and-assignment>>]

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's avatar
scott snyder committed
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's avatar
scott snyder committed
1006
String::~String()
1007
1008
1009
{ // correct behavior implemented in destructor
  delete m_data;
}
scott snyder's avatar
scott snyder committed
1010
1011
1012

...

1013
// declare and construct a ==> m_data points to "Hello"
scott snyder's avatar
scott snyder committed
1014
1015
1016
String a("Hello");

// open new scope
1017
{ // declare and construct b ==> m_data points to "World"
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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.* [<<self-assign>>]

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's avatar
scott snyder committed
1045

1046
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
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.*  [<<avoid-implicit-conversions>>]

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's avatar
scott snyder committed
1064

1065
1066
   In general, casts should be strongly discouraged, especially the old
   style C casts.
scott snyder's avatar
scott snyder committed
1067
1068
1069
1070
1071


 - *Use the C++ cast operators* (=dynamic_cast= *and* =static_cast=) *instead
    of the C-style casts.*  [<<use-c++-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's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
1087

1088
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1089
1090
1091
1092
1093
1094
1095
1096
1097
1098
1099
1100
1101
1102
1103
1104
1105
int n = 3;
double r = static_cast<double>(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<Base*>(d_ptr);
  // ...
}
#+END_EXAMPLE


 - *Do not convert const objects to non-const.*  [<<no-const-cast>>]

1106
   In general you should never cast away the constness of objects.
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
1124
1125
1126

 - *Do not use* =reinterpret_cast=. [<<no-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<float*>(buf);
  *fbuf = x;
  return *buf;
}
#+END_EXAMPLE

   the compiler is entitled to rewrite it as
scott snyder's avatar
scott snyder committed
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<float*>(buf);
  *fbuf = x;
  return ret;
}
#+END_EXAMPLE

   (As a special case, you can safely convert any pointer type to or from
1171
   a =char*=.)  The proper way to do such a conversion is with a union.
scott snyder's avatar
scott snyder committed
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.* [<<inline-functions>>]

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's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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).*  [<<large-argument-passing>>]
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
1210

1211
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1212
1213
1214
1215
1216
void func(char c);   // OK
void func(int i);    // OK
void func(double d); // OK
void func(complex<float> c); // OK

1217
void func(Track t); // not good, since Track is large, so
scott snyder's avatar
scott snyder committed
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's avatar
scott snyder committed
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's avatar
scott snyder committed
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.*  [<<modifiable-arguments>>]

    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.*
    [<<pass-ownership>>]

    To pass ownership of an object into a function, use =unique_ptr= 
    (by value):

    #+BEGIN_EXAMPLE
void foo (std::unique_ptr<Object> obj);

...

scott snyder's avatar
scott snyder committed
1259
  auto obj = std::make_unique<Object>();
1260
1261
1262
1263
1264
1265
1266
1267
1268
1269
1270
1271
1272
1273
1274
1275
1276
1277
  ...
  foo (std::move (obj);
#+END_EXAMPLE

    In most cases, =unique_ptr= should be passed by _value_.  There are
    however a few possible use cases for passing =unique_ptr= by reference:

     - The called function may replace the object passed in with another one.
       In this case, however, consider returning the new object as the
       value of the function.

     - The called function may only conditionally take ownership of the
       passed object.  This is likely to be confusing and error-prone
       and should probably be avoided.  Consider if a =shared_ptr=
       would be better in this case.

    There is basically no good case for passing =unique_ptr= as a const reference.

scott snyder's avatar
scott snyder committed
1278
1279
    If you need to interoperate with existing code, object ownership
    may be passed by pointer.
1280
1281
1282
1283
1284
1285
    The fact that ownership is transferred should be clearly documented.

    _Do not_ pass ownership using references.

    Here are a some additional examples to illustrate this.
    Assume that class =C= contains a member =Foo* m_owning_pointer=
1286
    which the class deletes.  (In modern C++, it would of course usually be better for
1287
1288
1289
1290
1291
1292
1293
1294
1295
1296
1297
1298
1299
1300
1301
1302
1303
1304
1305
1306
1307
1308
1309
1310
1311
    this to be a =unique_ptr=.)
    #+BEGIN_EXAMPLE
   // --- Best 
   void C::takesOwnership (std::unique_ptr<Foo> foo)
   {
      delete m_owning_pointer;
      m_owning_pointer = foo.release();
   }

   // --- Ok if documented.
   // Takes ownership of the @c foo pointer.
   void C::takesOwnership (Foo* foo)
   {
      delete m_owning_pointer;
      m_owning_pointer = foo;
   }

   // --- Don't do this!
   void C::takesOwnership (Foo& foo)
   {
      delete m_owning_pointer;
      m_owning_pointer = &foo;
   }
#+END_EXAMPLE

1312
 - *Return basic types or new instances of a class type by value*. [<<return-by-value>>]
1313
1314
1315
1316
1317
1318
1319
1320
1321
1322
1323
1324
1325
1326
1327
1328
1329
1330
1331
1332
1333
1334
1335
1336
1337
1338
1339
1340
1341

    Returning a class instance by value is generally preferred to passing
    an argument by non-const reference:

    #+BEGIN_EXAMPLE
// Bad
void getVector (std::vector<int>& v)
{
  v.clear();
  for (int i=0; i < 10; i++) v.push_back(v);
}

// Better
std::vector<int> getVector()
{
  std::vector<int> v;
  for (int i=0; i < 10; i++) v.push_back(v);
  return v;
}
#+END_EXAMPLE

    The return-value optimization plus move semantics will generally mean
    that there won't be a significant efficiency difference between the two.

 - *Use* =unique_ptr= *to return ownership.* [<<returning-ownership>>]

    If a function is returning a pointer to something that is allocated off the heap
    which the caller is responsible for deleting, then return a =unique_ptr=.

scott snyder's avatar
scott snyder committed
1342
    If compatibility with existing code is an issue,
1343
1344
1345
1346
1347
1348
1349
1350
1351
1352
1353
1354
1355
    then a plain pointer may be used, but the caller takes ownership should
    be clearly documented.

    _Do not_ return ownership via a reference.

    #+BEGIN_EXAMPLE
// Best
std::unique_ptr<Foo> makeFoo()
{
  return std::make_unique<Foo> (...);
}

// Ok if documented
1356
1357
// makeFoo() returns a newly-allocated Foo;
// caller must delete it.
1358
1359
1360
1361
1362
1363
1364
1365
1366
1367
1368
1369
Foo* makeFoo()
{
  return new Foo (...);
}

// NO!
Foo& makeFoo()
{
  Foo* foo = new Foo (...);
  return *foo;
}
#+END_EXAMPLE
scott snyder's avatar
scott snyder committed
1370
1371
1372
1373


 - *Have* =operator== *return a reference to* =*this=.  [<<assignment-return-value>>]

1374
    This ensures that
scott snyder's avatar
scott snyder committed
1375

1376
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1377
1378
1379
a = b = c;
#+END_EXAMPLE

1380
    will assign =c= to =b= and then =b= to =a= as is the case with built-in objects.
scott snyder's avatar
scott snyder committed
1381
1382
1383
1384
1385
1386
1387
1388


*** =const= correctness


 - *Declare a pointer or reference argument, passed to a function, as const if the function does not
    change the object bound to it.*  [<<const-arguments>>]

1389
1390
1391
    An advantage of =const=-declared parameters is that the compiler will
    actually give you an error if you modify such a parameter by mistake,
    thus helping you to avoid bugs in the implementation.
scott snyder's avatar
scott snyder committed
1392

1393
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1394
1395
1396
1397
1398
1399
1400
1401
// operator<< does not modify the String parameter
ostream& operator<<(ostream& out, const String& s);
#+END_EXAMPLE


 - *The argument to a copy constructor and to an assignment operator
   must be a const reference.* [<<copy-ctor-arg>>]

1402
    This ensures that the object being copied is not altered by the copy or assign.
scott snyder's avatar
scott snyder committed
1403
1404
1405
1406
1407


 - *In a class method, do not return pointers or non-const references to private data members.*
   [<<no-non-const-refs-returned>>]

1408
    Otherwise you break the principle of encapsulation.
scott snyder's avatar
scott snyder committed
1409

1410
    If necessary, you can return a pointer to a =const= or =const= reference.
scott snyder's avatar
scott snyder committed
1411

1412
1413
    This does not mean that you cannot have methods returning an =iterator=
    if your class acts as a container.
scott snyder's avatar
scott snyder committed
1414

1415
1416
1417
1418
    An allowed exception to this rule if the use of the singleton
    pattern.  In that case,
    be sure to add a clear explanation in a comment so
    that other developers will understand what you are doing.
scott snyder's avatar
scott snyder committed
1419
1420
1421
1422


 - *Declare as const a member function that does not affect the state of the object.* [<<const-members>>]

1423
1424
1425
    Declaring a member function as =const= has two important implications.
    First, only =const= member functions can be called for =const= objects;
    and second, a =const= member function will not change data members
scott snyder's avatar
scott snyder committed
1426

1427
1428
    It is a common mistake to forget to =const= declare member functions
    that should be =const=.
scott snyder's avatar
scott snyder committed
1429

1430
1431
1432
    This rule does not apply to the case where a member function which
    does not affect the state of the object overrides a non-const member
    function inherited from some super class.
scott snyder's avatar
scott snyder committed
1433
1434
1435
1436


 - *Do not let const member functions change the state of the program.* [<<really-const>>]

1437
1438
1439
1440
1441
1442
    A =const= member function promises not to change any of the data members
    of the object.  Usually this is not enough. It should be possible to
    call a =const= member function any number of times without affecting the
    state of the complete program. It is therefore important that a =const=
    member function refrains from changing static data members or other
    objects to which the object has a pointer or reference.
scott snyder's avatar
scott snyder committed
1443
1444
1445
1446
1447
1448

*** Overloading and default arguments

 - *Use function overloading only when methods differ in their argument list,
    but the task performed is the same.*