rules.org 107 KB
Newer Older
1
2
#+MACRO: version 0.6
#+LaTeX_HEADER: \def\rulesversion{0.6}
3
4
5
6
#+TITLE: ATLAS C++ coding guidelines, version {{{version}}}
#+AUTHOR: Shaun Roe (CERN), Scott Snyder (BNL), and the former ATLAS Quality Control group
#+EMAIL: Correspondence to snyder@bnl.gov.
# Checked with org-mode 8.2.10 (as bundled with emacs 25.2.1)
scott snyder's avatar
scott snyder committed
7

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

12
13
#+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
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
# 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}
36
37
38
#+LaTeX_HEADER: \rfoot{Version \rulesversion}

#+LaTeX_HEADER: \def\mylabel#1{\label{#1}#1}
scott snyder's avatar
scott snyder committed
39
40
41
42
43
44
45
46
47
48
49
50
51


* 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,
52
look up references on ``literate programming,'' such as [fn:Knuth84].)
scott snyder's avatar
scott snyder committed
53
54
55
56
57
58
59
60
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,
61
[[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
62
derived from work done by the CERN ``Project support team''
63
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
64
These previous guidelines have been significantly revised
65
to take into account the evolution of the C++ language [fn:Cxx14],
scott snyder's avatar
scott snyder committed
66
67
68
69
current practices in ATLAS, and the experience gained
over the past decade.

Some additional useful information on C++ programming may be
70
found in [fn:Meyers97], [fn:Meyers01], and [fn:GOF].
scott snyder's avatar
scott snyder committed
71
72
73
74
75
76
77
78
79
80
81
82
83
84

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

85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
   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
107
108
109
110
111
112
113
114
115
116
// 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>>]

117
   For example, =nameLength= is better than =nLn=.
scott snyder's avatar
scott snyder committed
118

119
120
   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
121
122
123
124


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

125
126
   In particular, avoid names that differ only in case.  For example,
   =track= / =Track=; =c1= / =cl=; =XO= / =X0=.
scott snyder's avatar
scott snyder committed
127
128
129
130
131
132
133
134
135


** 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>>]

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

138
139
   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
140
141
142
143
144
145
146
147


 - *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>>]

148
   Such names are reserved for the use of the compiler and system libraries.
scott snyder's avatar
scott snyder committed
149

150
151
152
153
154
   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
155
156
157
158
159
160
161
162
163
164


** 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>>]

165
   Use a lowercase letter after the prefix =s_=.
scott snyder's avatar
scott snyder committed
166
167
168

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

169
170
171
   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
172
173
174

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

175
176
177
178
179
180
   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
181

182
183
184
   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
185
186
187

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

188
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
189
190
191
192
193
194
195
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>>]

196
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
197
198
199
200
201
202
typedef vector<MCParticleKinematics*> TrackVector;
#+END_EXAMPLE

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

203
204
205
   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
206

207
  #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
208
209
210
211
212
213
typedef unsigned int mycounter_t;
#+END_EXAMPLE


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

214
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
215
216
217
218
double energy;
void extrapolate();
#+END_EXAMPLE

219
220
   Names starting with =s_= and =m_= should have a lowercase letter
   following the underscore.
scott snyder's avatar
scott snyder committed
221

222
223
224
   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
225
226
227
228

 - *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>>]

229
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
230
231
232
233
234
class OuterTrackerDigit;
double depositedEnergy;
void findTrack();
#+END_EXAMPLE

235
236
237
   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
238
239
240
241

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

242
243
244
   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
245
246
247

 - *Underscores should be avoided in package names.*  [<<no-underscores-in-package-names>>]

248
249
250
251
252
253
   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
254
255
256

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

257
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
258
259
260
261
METReconstruction, not MetReconstruction
MuonCSCValidation, not MuonCscValidation
#+END_EXAMPLE

262
   Unfortunately, existing code widely uses both forms.
scott snyder's avatar
scott snyder committed
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281


* 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>>]

282
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
283
284
285
286
287
288
#ifndef PACKAGE_CLASS_H
#define PACKAGE_CLASS_H
// The text of the header goes in here ...
#endif // PACKAGE_CLASS_H
#+END_EXAMPLE

289
290
291
   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
292

293
294
   The include guard should include both the package name and class name,
   to ensure that is unique.
scott snyder's avatar
scott snyder committed
295

296
   Don't start the include guard name with an underscore!
scott snyder's avatar
scott snyder committed
297

298
299
300
   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
   commented as such, and should usually have an extension other than ``.h''.
scott snyder's avatar
scott snyder committed
301
302
303

 - *Use forward declaration instead of including a header file,
    if this is sufficient.*  [<<forward-declarations>>]
304
305
   
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
306
307
308
309
310
311
312
313
314
class Line;
class Point
{
public:
  // Distance from a line
  Number distance(const Line& line) const;  
};
#+END_EXAMPLE

315
316
317
   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
318

319
320
   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
321
322
323
324

 - *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>>]

325
326
327
   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
328

329
330
331
332
333
334
335
336
337
   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
338
339
340
341

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

342
   This is for the same reason as for the previous item.
scott snyder's avatar
scott snyder committed
343
344
345

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

346
347
348
349
350
351
352
   =#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
353

354
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
355
356
357
358
359
360
361
362
363
364
365
366
367
368
// 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

369
370
371
   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
372

373
374
   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
375

376
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
377
378
379
380
381
382
// Obsolete --- don't do this anymore.
#ifndef MYPACKAGE_MYHEADER_H
# include "MyPackage/MyHeader.h"
#endif
#+END_EXAMPLE

383
384
385
   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
386
387
388

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

389
390
   A =using= directive or declaration imports names from one namespace 
   into another, often the global namespace.
scott snyder's avatar
scott snyder committed
391

392
393
394
395
396
   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
397

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

400
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
401
402
403
404
405
406
407
408
409
410
411
412
// 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

413
414
415
416
417
418
419
420
421
422
   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
423

424
425
426
427
   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
428

429
430
431
   Starting with C++11, =using= can also be used in ways similar to =typedef=.
   Such usage is not covered by this rule.

scott snyder's avatar
scott snyder committed
432
433
434
435
436

** Control flow

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

437
438
   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
439
440
   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
441

442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
 - *Prefer range-based for loops.*  [<<prefer-range-based-for>>]

   C++11 introduced the `range-based for'.  Prefer using this to a loop
   with explicit iterators; that is, prefer:
   #+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
467
   STL ranges are more simply written with a range-based for.
468
469


scott snyder's avatar
scott snyder committed
470
471
 - *All switch statements must have a default clause.*  [<<switch-default>>]

472
473
474
475
   In some cases the default clause can never be reached because there
   are case labels for all possible enum values in the switch statement,
   but by having such an unreachable default clause you show a potential
   reader that you know what you are doing.
scott snyder's avatar
scott snyder committed
476

477
478
479
480
481
   You also provide for future changes. If an additional enum value is
   added, the switch statement should not just silently ignore the new
   value. Instead, it should in some way notify the programmer that the
   switch statement must be changed; for example, you could throw an
   exception
scott snyder's avatar
scott snyder committed
482
483
484
485


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

486
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
// somewhere specified: enum Colors { GREEN, RED }

// semaphore of type Colors

switch(semaphore) {
case GREEN:
  // statement
  break;
case RED:
  // statement
  break;
default:
  // unforeseen color; it is a bug
  // do some action to signal it
}
#+END_EXAMPLE

504
505
506
507
   If you must ``fall through'' from one switch clause to another 
   (excluding the trivial case of a clause with no statements),
   this must be explicitly stated in a comment.  This should,
   however, be a rare case.
scott snyder's avatar
scott snyder committed
508

scott snyder's avatar
scott snyder committed
509
510
511
512
513
514
515
516
517
518
519
520
521
522
   #+BEGIN_EXAMPLE
switch (case) {
case 1:
  doSomething();
  /* FALLTHROUGH */
case 2:
  doSomethingMore();
  break;
...
#+END_EXAMPLE

   gcc7 will warn about such constructs unless you use a comment like
   in the example above.  (C++17 will add a =fallthrough= attribute.)

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

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

527
528
   This makes code much more readable and reliable, by clearly showing
   the flow paths.
scott snyder's avatar
scott snyder committed
529

530
531
532
   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
533

534
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
535
536
537
538
539
540
541
542
543
544
545
546
547
548
if (val == thresholdMin) {
  statement;
}
else if (val == thresholdMax) {
  statement;
}
else {
  statement;    // handles all other (unforeseen) cases
}
#+END_EXAMPLE


 - *Do not use goto.*  [<<no-goto>>]

549
   Use =break= or =continue= instead.
scott snyder's avatar
scott snyder committed
550

551
552
553
   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
554

555
556
   =goto= statements decrease readability and maintainability and make
   testing difficult by increasing the complexity of the code.
scott snyder's avatar
scott snyder committed
557

558
559
560
   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
561
562
563
564
565
566
567
568

** 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>>]

569
570
571
    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
572

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

576
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
577
578
579
580
581
582
583
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>>]

584
585
586
    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
587

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

590
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
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

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

608
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
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

629
630
631
632
633
634
635
636
637
    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
638

639
640
641
    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
642
643
644
645
646


 - *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>>]

647
648
    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
649

650
651
652
    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
653

654
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
655
656
657
658
659
660
661
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

662
663
    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
664

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



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

673
674
675
676
677
    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
678

679
680
681
682
683
684
685
686
687
688
 - *Be conservative in using* =auto=.  [<<using-auto>>]

   C++11 includes the new =auto= keyword, which allows one to omit explicitly
   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
689

690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
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
   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

   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
760
761
762
763
764
765
766


*** 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>>]

767
768
    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
769

770
771
772
773
    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
774

775
776
777
778
779
780
    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
781

782
783
784
    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
785

786
    Bad example:
scott snyder's avatar
scott snyder committed
787

788
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
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

807
    Correct 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
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

827
828
829
    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
830

831
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
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>>]

861
862
863
    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
864

865
    Bad example:
scott snyder's avatar
scott snyder committed
866

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

870
871
872
    #+BEGIN_EXAMPLE
Complex&
calculateC1 (const Complex& n1, const Complex& n2)
scott snyder's avatar
scott snyder committed
873
874
875
876
877
878
879
880
{
  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
881
  // the object is destroyed on exit from this function:
scott snyder's avatar
scott snyder committed
882
883
884
885
886
  // trouble ahead!
  return C1;
}
#+END_EXAMPLE

887
    In fact, most compilers will spot this and issue a warning.
scott snyder's avatar
scott snyder committed
888

889
890
    This particular function would be better written to return the result
    by value:
scott snyder's avatar
scott snyder committed
891

892
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
893
894
895
896
897
898
899
900
901
902
903
904
905
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>>]

906
907
908
    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
909

910
911
    By deleting the copy constructor and copy assignment operator, you can make a
    class non-copyable.
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
// There is only one ATLASExperimentalHall,
// and that should not be copied
class ATLASExperimentalHall
{
public:
  ATLASExperimentalHall();
  ~ATLASExperimentalHall();

  // Delete copy constructor --- disallow copying.
923
924
  ATLASExperimentalHall(const ATLASExperimentalHall& )
     = delete;
scott snyder's avatar
scott snyder committed
925
926

  // Delete assignment operator --- disallow assignment.
927
928
  ATLASExperimentalHall&
  operator=(const ATLAS_ExperimentalHall&) = delete;
scott snyder's avatar
scott snyder committed
929
930
931
};
#+END_EXAMPLE

scott snyder's avatar
scott snyder committed
932
933
    This syntax was new in C++11.  In C++98, this was achieved
    by declaring the deleted methods as private (and not implementing them).
scott snyder's avatar
scott snyder committed
934

935
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
// 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>>]

955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
    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
970
971
972
973
974
975
976
977
978
979
980
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)
981
982
983
{ // correct behavior implemented in constructor
  m_data = new char[strlen(value)]; // fill m_data
}
scott snyder's avatar
scott snyder committed
984
String::~String()
985
986
987
{ // correct behavior implemented in destructor
  delete m_data;
}
scott snyder's avatar
scott snyder committed
988
989
990

...

991
// declare and construct a ==> m_data points to "Hello"
scott snyder's avatar
scott snyder committed
992
993
994
String a("Hello");

// open new scope
995
{ // declare and construct b ==> m_data points to "World"
scott snyder's avatar
scott snyder committed
996
997
998
999
  String b("World");

  b=a;
  // execute default op= as synthesized by compiler ==>
1000
1001
  // memberwise assignment i.e. for pointers (m_data)
  //  bitwise copy
scott snyder's avatar
scott snyder committed
1002
1003
1004
1005
1006
1007
1008
1009
1010
  // ==> 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
}

1011
1012
// close scope: `b''s destructor called;
// memory still pointed to by `a' deleted!
scott snyder's avatar
scott snyder committed
1013
1014
1015
1016
1017
1018
1019
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>>]

1020
1021
1022
    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
1023

1024
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1025
1026
1027
1028
1029
1030
1031
1032
1033
1034
1035
1036
1037
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>>]

1038
1039
1040
1041
   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
1042

1043
1044
   In general, casts should be strongly discouraged, especially the old
   style C casts.
scott snyder's avatar
scott snyder committed
1045
1046
1047
1048
1049


 - *Use the C++ cast operators* (=dynamic_cast= *and* =static_cast=) *instead
    of the C-style casts.*  [<<use-c++-casts>>]

1050
1051
1052
   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
1053

1054
1055
1056
1057
1058
   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
1059

1060
1061
1062
1063
1064
   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
1065

1066
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1067
1068
1069
1070
1071
1072
1073
1074
1075
1076
1077
1078
1079
1080
1081
1082
1083
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>>]

1084
   In general you should never cast away the constness of objects.
scott snyder's avatar
scott snyder committed
1085

1086
1087
1088
1089
   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
1090

1091
1092
1093
1094
   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
1095

1096
1097
1098
1099
1100
1101
   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
1102
1103
1104

 - *Do not use* =reinterpret_cast=. [<<no-reinterpret-cast>>]

1105
1106
1107
1108
1109
1110
1111
1112
1113
1114
1115
1116
1117
1118
1119
1120
1121
1122
1123
1124
1125
1126
1127
1128
1129
1130
1131
1132
1133
1134
1135
1136
   =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
1137

1138
1139
1140
1141
1142
1143
1144
1145
1146
1147
1148
   #+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
1149
   a =char*=.)  The proper way to do such a conversion is with a union.
scott snyder's avatar
scott snyder committed
1150
1151
1152
1153
1154
1155
1156
1157
1158
1159


** 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>>]

1160
1161
1162
    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
1163

1164
1165
1166
1167
1168
1169
    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
1170

1171
1172
1173
    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
1174
1175
1176
1177
1178



*** Argument passing and return values

1179
1180
 -  *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
1181

1182
1183
1184
1185
1186
1187
    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
1188

1189
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1190
1191
1192
1193
1194
void func(char c);   // OK
void func(int i);    // OK
void func(double d); // OK
void func(complex<float> c); // OK

1195
void func(Track t); // not good, since Track is large, so
scott snyder's avatar
scott snyder committed
1196
1197
1198
                    // there is an overhead in copying t.
#+END_EXAMPLE

1199
1200
1201
1202
    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
1203

1204
1205
1206
1207
1208
    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
1209
1210
1211
void func(const LongString& s);  // const reference
#+END_EXAMPLE

1212
1213
1214
1215
1216
1217
1218
1219
1220
1221
1222
1223
1224
1225
1226
1227
1228
1229
1230
1231
1232
1233
1234
1235
1236

 -  *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
1237
  auto obj = std::make_unique<Object>();
1238
1239
1240
1241
1242
1243
1244
1245
1246
1247
1248
1249
1250
1251
1252
1253
1254
1255
  ...
  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
1256
1257
    If you need to interoperate with existing code, object ownership
    may be passed by pointer.
1258
1259
1260
1261
1262
1263
1264
1265
1266
1267
1268
1269
1270
1271
1272
1273
1274
1275
1276
1277
1278
1279
1280
1281
1282
1283
1284
1285
1286
1287
1288
1289
    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=
    which the class deletes.  (In C++11, it would of course usually be better for
    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

1290
 - *Return basic types or new instances of a class type by value*. [<<return-by-value>>]
1291
1292
1293
1294
1295
1296
1297
1298
1299
1300
1301
1302
1303
1304
1305
1306
1307
1308
1309
1310
1311
1312
1313
1314
1315
1316
1317
1318
1319

    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
1320
    If compatibility with existing code is an issue,
1321
1322
1323
1324
1325
1326
1327
1328
1329
1330
1331
1332
1333
1334
1335
1336
1337
1338
1339
1340
1341
1342
1343
1344
1345
1346
    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
// makeFoo() returns a newly-allocated Foo; caller must delete it.
Foo* makeFoo()
{
  return new Foo (...);
}

// NO!
Foo& makeFoo()
{
  Foo* foo = new Foo (...);
  return *foo;
}
#+END_EXAMPLE
scott snyder's avatar
scott snyder committed
1347
1348
1349
1350


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

1351
    This ensures that
scott snyder's avatar
scott snyder committed
1352

1353
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1354
1355
1356
a = b = c;
#+END_EXAMPLE

1357
    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
1358
1359
1360
1361
1362
1363
1364
1365


*** =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>>]

1366
1367
1368
    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
1369

1370
    #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1371
1372
1373
1374
1375
1376
1377
1378
// 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>>]

1379
    This ensures that the object being copied is not altered by the copy or assign.
scott snyder's avatar
scott snyder committed
1380
1381
1382
1383
1384


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

1385
    Otherwise you break the principle of encapsulation.
scott snyder's avatar
scott snyder committed
1386

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

1389
1390
    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
1391

1392
1393
1394
1395
    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
1396
1397
1398
1399


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

1400
1401
1402
    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
1403

1404
1405
    It is a common mistake to forget to =const= declare member functions
    that should be =const=.
scott snyder's avatar
scott snyder committed
1406

1407
1408
1409
    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
1410
1411
1412
1413


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

1414
1415
1416
1417
1418
1419
    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
1420
1421
1422
1423
1424
1425

*** Overloading and default arguments

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

1426
1427
1428
    Using function name overloading for any other purpose than to group
    closely related member functions is very confusing and is not
    recommended.
scott snyder's avatar
scott snyder committed
1429
1430
1431
1432
1433
1434
1435

** =new= and =delete=


 - *Do not use new and delete where automatic allocation will work.* 
   [<<auto-allocation-not-new-delete>>]

1436
1437
1438
1439
1440
1441
   Suppose you have a function that takes as an argument a pointer to an object,
   but the function does not take ownership of the object.  Then suppose you
   need to create a temporary object to pass to this function.  In this case,
   it's better to create an automatically-allocated object on the stack
   than it is to use =new= / =delete=.  The former will be faster, and you
   won't have the chance to make a mistake by omitting the =delete=.
scott snyder's avatar
scott snyder committed
1442

1443
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1444
1445
1446
1447
1448
1449
1450
1451
1452
1453
1454
1455
1456
1457
1458
  // Not good:
  Foo* foo = new Foo;
  doSomethingWithFoo (foo);
  delete foo;

  // Better:
  Foo foo;
  doSomethingWithFoo (&foo);
#+END_EXAMPLE



 - *Match every invocation of new with one invocation of delete in all
   possible control flows from new.* [<<match-new-delete>>]

1459
   A missing delete would cause a memory leak.
scott snyder's avatar
scott snyder committed
1460

1461
1462
1463
   However, in the Gaudi/Athena framework, an object created with
   =new= and registered in StoreGate is under
   control of StoreGate and must not be deleted.
scott snyder's avatar
scott snyder committed
1464

scott snyder's avatar
scott snyder committed
1465
   In new code, you should generally use =make_unique= for this.
scott snyder's avatar
scott snyder committed
1466

1467
   #+BEGIN_EXAMPLE
scott snyder's avatar
scott snyder committed
1468
1469
1470
1471
#include <memory>

  ...
  DataVector<C>* dv = ...;
scott snyder's avatar
scott snyder committed
1472
  auto c = std::make_unique<C>("argument");
scott snyder's avatar
scott snyder committed
1473
1474
  ...
  if (test) {
1475
    dv->push_back (std::move (c));
scott snyder's avatar
scott snyder committed
1476
1477
1478
  }
#+END_EXAMPLE

1479
1480
1481
   =auto_ptr= was an attempt to do something similar to =unique_ptr= in older
   versions of the language.  However, it has some serious deficiencies
   and should not be used in new code.
scott snyder's avatar
scott snyder committed
1482
1483
1484
1485
1486


 - *A function should explicitly document if it takes ownership of a
   pointer passed to it as an argument.*  [<<explicit-ownership>>]