Basic C++ tone generator

Question

I'm trying to learn C++ (with experience in C and Java, inter alia). I wrote a program to output a waveform of multiple (superimposed) tones, each varying in pitch.

To execute:

• Compilation: g++ -W -Wall -pedantic -std=c++11 main.cpp Tone.cpp
• Running: ./a.out
• Playing: aplay out -r 44100 -f S16_LE (or S16_BE if you're on a big-endian system)

What can I improve in terms of C++ style and idioms? I tried to use RAII for my streams and collections, and I tried to avoid news and pointers.

A couple specific questions:

• In write_sample, are the cast to (char *) and the sizeof divisions okay? If not, what's the standard C++ way to write binary data to a file? Is sizeof really even used in C++, or is this a discouraged C artifact?
• What should be const vs. constexpr?
• I used a C-style array for octaves in main, instead of std::array<double, 7>, because I like the automatic initializer counting and the iteration is easier. Is this okay? What would be better?
• Does C++ use size_t, int16_t, and the like?

---

Tone.hpp

#ifndef TONE_H
#define TONE_H
class Tone
{
 private:
 const double sampleRate;
 double s; /* current position along the unit sine curve */
 double lastSample;
 public:
 double getSample() const;
 double nextSample(double frequency, double amplitude);
 Tone(double sampleRate);
};
#endif

Tone.cpp

#include <cmath>
#include "Tone.hpp"
Tone::Tone(double sampleRate) : sampleRate(sampleRate), s(0), lastSample(0)
{
}
double Tone::getSample() const
{
 return this->lastSample;
}
double Tone::nextSample(double frequency, double amplitude)
{
 this->s += frequency / this->sampleRate;
 this->lastSample = amplitude * sin(2*M_PI * this->s);
 return this->lastSample;
}

main.cpp

#include <cmath>
#include <vector>
#include <iostream>
#include <fstream>
#include "Tone.hpp"
void write_sample(std::ostream &stream, double sample);
int main()
{
 constexpr double SAMPLE_RATE = 44100;
 std::vector<Tone> tones;
 // Each of these represents a final octave difference from A4
 const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 const size_t count = sizeof(octaves) / sizeof(*octaves);
 std::ofstream outfile("out", std::ofstream::binary);
 // Create a tone generator for each
 for (size_t i = 0; i < count; i++)
 {
 tones.push_back(Tone(SAMPLE_RATE));
 }
 // Set parameters for the output sound
 double duration = 4.0;
 double attack = 1.0;
 double decay = 2.5;
 double amplitude = 0.8;
 // Generate the required samples
 for (double t = 0; t < duration; t += 1 / SAMPLE_RATE)
 {
 // Interpolate with sinusoidal easing up to the attack time, then constant
 double interp = t >= attack ? 1 : sin(M_PI/2 * t);
 // Constant until the decay time, then interpolate linearly out
 double strength = t < (duration - decay) ? 1
 : 1 - (t - (duration - decay)) / decay;
 // Superimpose the samples from each tone generator
 double totalSample = 0;
 for (size_t i = 0; i < count; i++)
 {
 double octave = interp * octaves[i];
 double frequency = 440 * pow(2, octave);
 double sample = tones[i].nextSample(frequency, strength * amplitude);
 totalSample += sample;
 }
 totalSample /= count;
 if (outfile.good())
 {
 write_sample(outfile, totalSample);
 }
 else
 {
 return 1;
 }
 }
 return 0;
}
// Writes a sample to the given output stream as a 16-bit signed integer.
void write_sample(std::ostream &stream, double sample)
{
 int16_t output = (int16_t) (0x7FFF * sample);
 stream.write((char *) &output, sizeof(int16_t) / sizeof(char));
}

Answer 1

Overall it looks fine for a newcomer, you've made proper use of const for variables and methods, which is something beginners tend to overlook.

A few points you can still work on:

1. For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. That would be less boilerplate and easier to maintain. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (like get/set accessors), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables. I don't recommend it as an optimization either, the gain is in reducing redundant/boilerplate code implicit in the prototype/implementation setup.

2. Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely identify your variables).

3. This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.

4. When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.

5. Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.

6. Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.

7. Your main() function is too long and doing too much. You should probably have another class, Sample perhaps, that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.

8. There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

   std::vector<Tone> tones(count, Tone(SAMPLE_RATE));
   

9. If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

   // Length in elements of type 'T' of statically allocated C++ arrays.
   template<class T, std::size_t N>
   constexpr std::size_t arrayLength(const T (&)[N])
   {
    return N;
   }
   ...
   const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
   const auto count = arrayLength(octaves);
   

   constexpr will force the function to be evaluated at compile time, thus making it suitable for use in places a compile-time constant would be required.

• wchargin: Thanks! These were just the type of things I was hoping to see. A few questions. Will (8) create separate tones for each, or just copy the same object to each slot? And how does (9) play into actually declaring the std::array—would you then write std::array<double, count> realOctaves = octaves or something?
• glampert: @WChargin, the "fill" constructor of vector will have the same effects of your for loop. It will produce a vector with count instances of Tone(SAMPLE_RATE).
• glampert: @WChargin, with array, you will have to explicitly write the size on declaration. E.g.: const std::array<double, 7> octaves = { ... };. It's a tradeoff, no size inference on initialization but you get the size() method afterwards that gives you the array length, which removes the need for separate variables, plus a few other nice things like iterators...
• fluffy: Thanks to modern compilers' support of link-time optimization I no longer recommend that people do header-inlined code unless it's truly trivial, as it is no longer a performance improvement.
• glampert: @fluffy, yes, I would not suggest that as an optimization either. The only reason would be the convenience of unifying declaration and implementation, but of course, only viable for trivial functions.

Answer 2

What @glampert said:

Dead Code

double Tone::getSample() const
{
 return this->lastSample;
}

This is never used. Also looks like a dreaded getter so probably don't want it anyway.

Easier iteration

const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
const size_t count = sizeof(octaves) / sizeof(*octaves);
 for (size_t i = 0; i < count; i++)
 {
 double octave = interp * octaves[i];
 // STUFF
 }

I would say it is easier to use the C++ std::Array (or vector)

 std::vector<double> const octaves = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
 for(double octVal : octaves)
 {
 double octave = interp * octVal;
 // STUFF
 }

Prefer Emplace to Push

// Create a tone generator for each
for (size_t i = 0; i < count; i++)
{
 tones.push_back(Tone(SAMPLE_RATE));
}

This creates a temporary object then copy constructs it into the tones vector (you may get a move if you have a move constructor). But you can get the vecotr to build the object in place rather than creating a temporary then copying by using emplace.

// Create a tone generator for each
for (size_t i = 0; i < count; i++)
{
 tones.emplace_back(SAMPLE_RATE); // You pass all the arguments the constructor takes
 // Even if this is more than one. The emplace_back
 // forwards all the parameters to the constructor
 // and does an inplace create in the vector.
}

No need to check this every iteration of the loop;

 if (outfile.good())
 {
 write_sample(outfile, totalSample);
 }
 else
 {
 return 1;
 }
}
return 0;

If the stream has gone bad then writing to it will not do anything. Also a stream going bad is a very expcetional thing so I would just test for it at the end of the function not every time through the loop.

I would simplify the above code too:

 write_sample(outfile, totalSample); 
}
return outfile.good() ? 0 : 1;

C Style cast

Don't use them. They are hard to find and the cause of a lot of errors. Use the C++ cast (which are easy to find) and are slightly more nuanced.

int16_t output = (int16_t) (0x7FFF * sample);

Don't believe that line actually needs a cast. If it warns you about loosing precision then by then you should use a cast (to let the compiler know you know what you are doing and are willing to loose precision) but it should be a static_cast<int16_t>()

• fluffy: Oh wow, I didn't know about emplace_back. +1 just for that. (All the other stuff gets +1 too.)
• wchargin: Thanks for all your suggestions. Could you explain why getters are "dreaded"? What's the proper way to readonly-expose a field?
• wchargin: Also, I don't think I can use the foreach iteration because I need to access both octaves[i] and tones[i]. I don't want to have to include Boost just to get a zip function—though if those are built in now, that'd be perfect (e.g., in Python it'd be for octave, tone in zip(octaves, tones): pass).
• Loki Astari: @WChargin: Getters: Its all situational so giving a general solution is going to have holes when you argue a specific situation. But if you are exposing a member (even read only) I would ask why (there could be a good reason but usually that is an indication of a design flaw). You should be exposes verbs based methods that manipulate the class's internal state.
• Loki Astari: @WChargin: Boost: Though the best stuff in boost has already been moved into the standard. But there are still good things in there. You should really consider boost as a test bed for the standard and have it installed anyway (it saves so much time in all projects (don't think of it terms of just this project install it into the system)).