C++ best practices¶

The examples here do not rely on an Athena release. However, they do require a modern C++ compiler. If unsure, you can always set up a recent Athena release anyway, in order to pick up the compiler from there.

For these examples, you should compile with C++20 enabled, so add -std=c++20 to your command line. For (at least) the examples where timing measurements are compared, you should also turn on optimization with -O2.

To start the exercise, download ex1a.cpp or copy and paste it from the "Exercise 1" block immediately under this text. The solutions to each of the problems below, in the form of modified versions of this example, are collected in the tar file ex1.tar (but try to do the exercises yourself first).

Exercise 1

ex1a.cpp

#include <vector>
#include <cmath>
#include <cstdlib>
#include <iostream>
#include <map>
#include <cassert>
#include <stdexcept>
#include <chrono>
#include <format>
#include <stdint.h>


static const double GeV = 1000;


//************************************************************************
// Very simple random number generator.


/// Generate a floating-point random number between @c rmin and @c rmax.
float randf (float rmax, float rmin = 0)
{
  static constexpr uint32_t rngmax = static_cast<uint32_t> (-1);
  static uint32_t seed = 1;
  seed = (1664525*seed + 1013904223);
  return static_cast<float>(seed) / static_cast<float>(rngmax) * (rmax-rmin) + rmin;
}


/// Generate an integer random number between @c rmin and @c rmax.
int randi (int rmax, int rmin = 0)
{
  return static_cast<int> (randf (rmax, rmin));
}


//************************************************************************
// Timer class.


class Timer
{
public:
  using clock_t = std::chrono::steady_clock;
  using time_point_t = std::chrono::time_point<clock_t>;
  using duration_t = std::chrono::duration<float>;

  void start (const char* name);
  void stop (const char* name);
  void dump();

private:
  struct Timeent {
    duration_t elapsed;
    time_point_t starttime;
    bool running = false;
  };

  std::map<std::string, Timeent> m_timers;
};


void Timer::start (const char* name)
{
  Timeent& ent = m_timers[name];
  //assert (!ent.running);
  ent.starttime = clock_t::now();
  ent.running = true;
}


void Timer::stop (const char* name)
{
  Timeent& ent = m_timers[name];
  //assert (ent.running);
  ent.elapsed += clock_t::now() - ent.starttime;
  ent.running = false;
}


void Timer::dump()
{
  for (const auto& x : m_timers) {
    std::cout << std::format ("{:5} {:.02f}\n", x.first, x.second.elapsed.count());
  }
}


Timer timers;



//************************************************************************


class Fourvec
{
public:
  Fourvec (double x = 0, double y = 0, double z = 0);
  Fourvec (const Fourvec& other);
  Fourvec& operator= (const Fourvec&) = default;
  ~Fourvec();
  double x() { return m_x; }
  double y() { return m_y; }
  double z() { return m_z; }
  double e() { return m_e; }

  Fourvec& operator+= (Fourvec& other);


  static int s_count;

private:
  double m_x;
  double m_y;
  double m_z;
  double m_e;
};


int Fourvec::s_count = 0;



Fourvec::Fourvec (double x, double y, double z)
  : m_x (x), m_y (x), m_z (z), m_e (std::sqrt (x*x + y*y + z*z))
{
  ++s_count;
}


Fourvec::Fourvec (const Fourvec& other)
{
  ++s_count;
  m_x = other.m_x;
  m_y = other.m_y;
  m_z = other.m_z;
  m_e = other.m_e;
}


Fourvec::~Fourvec() { --s_count; }

Fourvec& Fourvec::operator+= (Fourvec& other)
{
  m_x += other.m_x;
  m_y += other.m_y;
  m_z += other.m_z;
  m_e += other.m_e;
  return *this;
}


typedef Fourvec Track;


class Cluster
  : public Fourvec
{
public:
  Cluster (const Fourvec& v, Track* track = 0);
  ~Cluster();

private:
  Track* m_track;
};



Cluster::Cluster (const Fourvec& v, Track* track)
  : Fourvec (v),
    m_track (track)
{
}


Cluster::~Cluster()
{
  delete m_track;
}



typedef std::vector<Cluster> ClusterVec;


//************************************************************************


Fourvec randvec()
{
  return Fourvec (randf(100*GeV,-100*GeV),
                  randf(100*GeV,-100*GeV),
                  randf(100*GeV,-100*GeV));
}


Cluster makeCluster()
{
  Track* track = 0;
  Fourvec v = randvec();
  if (randi(2) == 0)
    track = new Track (v.x() * 0.9, v.y() * 0.9, v.z() * 0.9);

  if (randi(1000000) == 0)
    throw std::runtime_error("asd");
  return Cluster (v, track);
}


ClusterVec makeClusters()
{
  ClusterVec vec;
  for (int i = 0; i < 1000; i++)
    vec.push_back (makeCluster());
  return vec;
}


Fourvec sumClusters (ClusterVec& vec)
{
  Fourvec sum;
  ClusterVec::iterator beg = vec.begin();
  ClusterVec::iterator end = vec.end();
  for (; beg != end; ++beg)
    sum += *beg;
  return sum;
}


double doit()
{
  timers.start ("doit");
  ClusterVec vec = makeClusters();
  Fourvec sum = sumClusters (vec);
  timers.stop ("doit");
  return std::hypot (sum.x(), sum.y());
}


double doit2()
{
  timers.start ("doit2");
  Fourvec sum;
  for (int i=0; i < 1000; i++) {
    Fourvec* vec = new Cluster (makeCluster());
    sum += *vec;
    delete vec;
  }
  timers.stop ("doit2");
  return std::hypot (sum.x(), sum.y());
}


int main (int argc, char** argv)
{
  int n = 100000;
  if (argc > 1)
    n = std::atoi (argv[1]);

  double sum1 = 0;
  double sum2 = 0;
  for (int i=0; i < n; i++) {
    try {
      sum1 += doit();
      sum2 += doit2();
    }
    catch (const std::runtime_error&) {
    }
  }

  std::cout << sum1 << " " << sum2 << "\n";
  std::cout << "Objects leaked: " << Fourvec::s_count << "\n";
  timers.dump();
  return 0;
}

The first couple sections you don't need to look at closely now: there are some functions for generating random numbers and a timer class. Below that, we define a very simple Fourvec class and also assign it a name Track. We also define a Cluster class that derives from Fourvec and also optionally owns a pointer to a Track. This pointer is passed to the constructor, and is deleted in the destructor.

Below this, there's a method makeCluster() which creates a Cluster== object initialized with random values, with a 50% chance of having an associated Track. This function can also very rarely raise an exception. makeClusters() then uses this to set up a vector of these objects. sumClusters() then performs a simple calculation from these objects, and doit() puts together makeClusters() and sumClusters() along with a timer. doit2() does something similar, but it allocates the clusters dynamically. The outer loop in the main function catches and ignores exceptions.

That's what it's supposed to do, anyway. However, if you actually compile and run it, it crashes:
```
$ g++ -O2 -o ex1a -std=c++20 -Wall -Wextra ex1a.cpp
$ ./ex1a
free(): double free detected in tcache 2
Aborted (core dumped)
```
Why?

Answer

Yes, the problem is the Cluster class: it owns a pointer and we copy them, but we use the defaulted copy constructor. Supply a proper copy constructor and assignment operator for Cluster and then rerun. The solution is in ex1b.cpp in the tarball.
After that, I get output like this:
```
2.05621e+11 2.05306e+11
Objects leaked: 49930318
doit  7.14
doit2 3.93
```
We have a bunch of objects leaked. Do you see where that's happening?
Answer

It's in doit2():
```
Fourvec* vec = new Cluster (makeCluster());
sum += *vec;
delete vec;
```
We create a Cluster object but delete it as a Fourvec. Since Fourvec has a non-virtual destructor, the Cluster destructor doesn't run, and the Track pointer doesn't get deleted.

Make the Fourvec destructor virtual and try again. Remember to also declare the Cluster destructor as virtual. The solution is in ex1c.cpp in the tarball.
Now I get something that looks like this:
```
2.05621e+11 2.05306e+11
Objects leaked: 92
doit  6.43
doit2 4.10
```
Better --- but we still have some objects leaked. Where are they coming from?
Answer

In makeCluster(), if we raise an exception, then a Track object may be leaked.
```
if (randi(2) == 0)
    track = new Track (v.x() * 0.9, v.y() * 0.9, v.z() * 0.9);

if (randi(1000000) == 0)
    throw std::runtime_error("asd");
```
Rewrite the code to use unique_ptr throughout where appropriate. You'll probably find that this simplifies things. It also automatically takes care of this memory leak.

Remember to use =std::move= when you want a named pointer object to give up ownership. The solution is in ex1d.cpp in the tarball.
With that change, the memory leaks are gone. Now, in the timer class, there are two assertions that are commented out, in the start() and top() functions:
```
//assert (!ent.running);
//assert (ent.running);
```
Try removing the comment marks so the lines become active, and you'll see why they were commented out. The program crashes. Why?
Answer

It's the exceptions again. In this sequence:
```
timers.start ("doit");
ClusterVec vec = makeClusters();
Fourvec sum = sumClusters (vec);
timers.stop ("doit");
```
If makeClusters() raises an exception, then we'll have a call to start() with no corresponding call to stop(). This triggers the assertion. Rewrite this using RAII. The solution can be found in the code ex1e.cpp in the tarball.
Now let's take a quick look at a performance issue. We have this inside an inner loop:
```
for (int i = 0; i < 1000; i++) vec.push_back (makeCluster());
```
Since the object returned by makeCluster() is a temporary, this can be done by a move. But since we didn't supply a move constructor for Cluster that means that this is actually being done by a copy. For a Cluster holding a Track object, that means that we make a new Track object, copy from the original, then delete the original.

Since we're now holding the =Track= object via a unique_ptr, the default move constructor will in fact work. You just need to tell the compiler to use it:
```
Cluster(Cluster&& other) = default;
```
This changes the timings i see from
```
doit  5.81
doit2 3.62
```
to
```
doit  5.22
doit2 2.93
```
In makeClusters(), you can also try to add a reserve() for the vector before filling it. This brings the timing down to
```
doit  3.22
doit2 2.90
```
The solution is in ex1f.cpp in the tarball.

The sumClusters() method is implemented like this:

Fourvec sum;
ClusterVec::iterator beg = vec.begin();
ClusterVec::iterator end = vec.end();
for (; beg != end; ++beg)
    sum += *beg;
return sum;

Rewrite this using a range-based for. The solution is in ex1g.cpp in the tarball.

There are numerous declarations in this code that should be const but aren't. Fix them. The solution is in ex1h.cpp in the tarball.

Original Twiki version