Last Saturday, a colleague asked me to help look at a piece of code that was behaving strangely. To simplify the explanation, I have distilled the core logic as follows:
#include <algorithm>#include <iostream>#include <iterator>
#define FS 20 // Sampling rate#define SECS 5 // Seconds#define BUF_LEN FS*SECS // Buffer length#define CHANNELS 5 // Number of channels
class Buffer{public: Buffer() : size(0) { std::fill(std::begin(buffer), std::end(buffer), 0); }
void Add(int v) { buffer[(size++) % BUF_LEN] = v; }
const int *GetData() const { return buffer; }
int GetSize() const { return size; }
private: int buffer[BUF_LEN]; int size;};
std::ostream &operator<<(std::ostream &os, const Buffer &buffer){ for (int i = 0; i < buffer.GetSize(); ++i) os << buffer.GetData()[i] << ' ';
return os;}
int main(){ Buffer *channelBuffers = new Buffer[CHANNELS];
for (int c = 0; c < CHANNELS; ++c) { for (int i = 0; i < 5; ++i) channelBuffers[c].Add(c + 1);
std::cout << channelBuffers[c] << std::endl; }
delete[] channelBuffers;
return 0;}
The logic of this code is quite simple:
-
Allocate a data buffer for each channel;
-
Write five
<span>1</span>to the first channel, five<span>2</span>to the second channel, and so on, with five<span>5</span>to the fifth channel.
Intuitively, we expect the output to be:
1 1 1 1 12 2 2 2 23 3 3 3 34 4 4 4 45 5 5 5 5
However, the actual output is:
1 0 0 0 02 0 0 0 03 0 0 0 04 0 0 0 05 0 0 0 0
It seems that each line only writes once, and the subsequent values “disappear into thin air.” Did they really get lost? To further verify, we slightly modified the code to output more content from the buffer:
#include <algorithm>#include <iostream>#include <iterator>
#define FS 20 // Sampling rate#define SECS 5 // Seconds#define BUF_LEN FS*SECS // Buffer length#define CHANNELS 5 // Number of channels
class Buffer{public: Buffer() : size(0) { std::fill(std::begin(buffer), std::end(buffer), 0); }
void Add(int v) { buffer[(size++) % BUF_LEN] = v; }
const int *GetData() const { return buffer; }
int GetSize() const { return size; }
private: int buffer[BUF_LEN]; int size;};
struct BufferPrinter{ const Buffer &buffer; int len;};
std::ostream &operator<<(std::ostream &os, const BufferPrinter &buffer){ auto len = std::min(BUF_LEN, buffer.len); for (int i = 0; i < len; ++i) os << buffer.buffer.GetData()[i] << ' ';
return os;}
int main(){ Buffer *channelBuffers = new Buffer[CHANNELS];
for (int c = 0; c < CHANNELS; ++c) { for (int i = 0; i < 5; ++i) channelBuffers[c].Add(c + 1);
std::cout << BufferPrinter{ channelBuffers[c], 25 } << std::endl; }
delete[] channelBuffers; return 0;}
Output results as follows:
1 0 0 0 0 1 0 0 0 0 1 0 0 0 0 1 0 0 0 0 1 0 0 0 02 0 0 0 0 2 0 0 0 0 2 0 0 0 0 2 0 0 0 0 2 0 0 0 03 0 0 0 0 3 0 0 0 0 3 0 0 0 0 3 0 0 0 0 3 0 0 0 04 0 0 0 0 4 0 0 0 0 4 0 0 0 0 4 0 0 0 0 4 0 0 0 05 0 0 0 0 5 0 0 0 0 5 0 0 0 0 5 0 0 0 0 5 0 0 0 0
It turns out that the data was not lost, but rather written to unexpected locations.
Can you spot where the problem lies? If not, don’t worry, the answer will be revealed later.
Now, let’s reveal the answer. Let’s focus on these lines of code:
#define FS 20 // Sampling rate#define SECS 5 // Seconds#define BUF_LEN FS*SECS // Buffer length
class Buffer{public: void Add(int v) { buffer[(size++) % BUF_LEN] = v; }
private: int buffer[BUF_LEN]; int size;};
The issue lies in the implementation of Buffer.Add.
In C and C++, macro definitions are expanded during the preprocessing stage, and this expansion can be viewed as simple text replacement. Thus:
buffer[(size++) % BUF_LEN] = v;
will first expand to:
buffer[(size++) % FS*SECS] = v;
Further replacement yields:
buffer[(size++) % 20*5] = v;
Therefore, during the first five iterations, the value of the expression <span>(size++) % 20*5</span> is sequentially: 0, 5, 10, 15, 20.
So, how can we avoid this issue?
First: Avoid using macro definitions. In C++, you can use constants or constexpr (C++11) to replace macro definitions. For example:
const int FS = 20; // Sampling rateconst int SECS = 5; // Secondsconst int BUF_LEN = FS * SECS; // Buffer lengthconst int CHANNELS = 5; // Number of channels
constexpr int FS = 20; // Sampling rateconstexpr int SECS = 5; // Secondsconstexpr int BUF_LEN = FS * SECS; // Buffer lengthconstexpr int CHANNELS = 5; // Number of channels
If you must use macros, you can also add parentheses to the macro definitions to make the expansion safer:
#define FS 20 // Sampling rate#define SECS 5 // Seconds#define BUF_LEN ((FS)*(SECS)) // Buffer length
Thus, after expansion, it becomes:
buffer[(size++) % ((20)*(5))] = v;
Which meets expectations.