Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Beats: Use std::make_shared/shared_from_this() #4336

Merged
merged 7 commits into from
Oct 1, 2021
Merged

Beats: Use std::make_shared/shared_from_this() #4336

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6cb1da2
Beats: Add missing const qualifiers
uklotzde Sep 29, 2021
1bbee5f
Beats: Delete obsolete comments
uklotzde Sep 29, 2021
df574b1
Beats: Use std::enable_shared_from_this
uklotzde Sep 29, 2021
beaf525
Beats: Use std::make_shared
uklotzde Sep 29, 2021
27be226
Beats: Unify behavior of scale()/translate() operations
uklotzde Sep 29, 2021
91b5d8c
Beats: Add redundant type name
uklotzde Sep 30, 2021
90d362b
Beats: Rename clone() function
uklotzde Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 33 additions & 26 deletions src/track/beatgrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class BeatGridIterator : public BeatIterator {
};

BeatGrid::BeatGrid(
MakeSharedTag,
audio::SampleRate sampleRate,
const QString& subVersion,
const mixxx::track::io::BeatGrid& grid,
Expand All @@ -54,21 +55,20 @@ BeatGrid::BeatGrid(
m_sampleRate(sampleRate),
m_grid(grid),
m_beatLengthFrames(beatLengthFrames) {
// BeatGrid should live in the same thread as the track it is associated
// with.
}

BeatGrid::BeatGrid(const BeatGrid& other,
BeatGrid::BeatGrid(
MakeSharedTag,
const BeatGrid& other,
const mixxx::track::io::BeatGrid& grid,
audio::FrameDiff_t beatLengthFrames)
: m_subVersion(other.m_subVersion),
m_sampleRate(other.m_sampleRate),
m_grid(grid),
m_beatLengthFrames(beatLengthFrames) {
: BeatGrid({}, other.m_sampleRate, other.m_subVersion, grid, beatLengthFrames) {
}

BeatGrid::BeatGrid(const BeatGrid& other)
: BeatGrid(other, other.m_grid, other.m_beatLengthFrames) {
BeatGrid::BeatGrid(
MakeSharedTag,
const BeatGrid& other)
: BeatGrid({}, other, other.m_grid, other.m_beatLengthFrames) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you don't pass the MakeSharedTag, but create a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because type tags are not supposed to carry any information and should be eliminated during compilation. Passing the instance is not desired in this special case. Just a hint for the compiler to ignore the unnamed parameter.

}

// static
Expand Down Expand Up @@ -98,7 +98,8 @@ BeatsPointer BeatGrid::makeBeatGrid(
// Calculate beat length as sample offsets
const audio::FrameDiff_t beatLengthFrames = 60.0 * sampleRate / bpm.value();

return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
return std::make_shared<BeatGrid>(
MakeSharedTag{}, sampleRate, subVersion, grid, beatLengthFrames);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use here the explicit version, where above you use only the braced tantalizer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because std::make_shared() is not able to deduce the type and compilation fails. Try it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be explicit everywhere.

}

// static
Expand All @@ -109,12 +110,16 @@ BeatsPointer BeatGrid::fromByteArray(
mixxx::track::io::BeatGrid grid;
if (grid.ParseFromArray(byteArray.constData(), byteArray.length())) {
const audio::FrameDiff_t beatLengthFrames = (60.0 * sampleRate / grid.bpm().bpm());
return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
return std::make_shared<BeatGrid>(MakeSharedTag{},
sampleRate,
subVersion,
grid,
beatLengthFrames);
}

// Legacy fallback for BeatGrid-1.0
if (byteArray.size() != sizeof(BeatGridData)) {
return BeatsPointer(new BeatGrid(sampleRate, QString(), grid, 0));
return std::make_shared<BeatGrid>(MakeSharedTag{}, sampleRate, QString(), grid, 0);
}
const BeatGridData* blob = reinterpret_cast<const BeatGridData*>(byteArray.constData());
const auto firstBeat = mixxx::audio::FramePos(blob->firstBeat);
Expand Down Expand Up @@ -248,27 +253,28 @@ mixxx::Bpm BeatGrid::getBpm() const {
mixxx::Bpm BeatGrid::getBpmAroundPosition(audio::FramePos position, int n) const {
Q_UNUSED(position);
Q_UNUSED(n);

if (!isValid()) {
return {};
}
return bpm();
return getBpm();
}

BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
if (!isValid()) {
return BeatsPointer(new BeatGrid(*this));
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clone();
}

mixxx::track::io::BeatGrid grid = m_grid;
const audio::FramePos newFirstBeatPosition = firstBeatPosition() + offset;
grid.mutable_first_beat()->set_frame_position(
static_cast<google::protobuf::int32>(
newFirstBeatPosition.toLowerFrameBoundary().value()));

return BeatsPointer(new BeatGrid(*this, grid, m_beatLengthFrames));
return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, m_beatLengthFrames);
}

BeatsPointer BeatGrid::scale(BpmScale scale) const {
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clone();
}

mixxx::track::io::BeatGrid grid = m_grid;
auto bpm = mixxx::Bpm(grid.bpm().bpm());

Expand All @@ -293,27 +299,28 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
break;
default:
DEBUG_ASSERT(!"scale value invalid");
return BeatsPointer(new BeatGrid(*this));
return clone();
}

if (!bpm.isValid()) {
return BeatsPointer(new BeatGrid(*this));
return clone();
}

bpm = BeatUtils::roundBpmWithinRange(bpm - kBpmScaleRounding, bpm, bpm + kBpmScaleRounding);
grid.mutable_bpm()->set_bpm(bpm.value());
const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * m_sampleRate / bpm.value());
return BeatsPointer(new BeatGrid(*this, grid, beatLengthFrames));
return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
}

BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) {
BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) const {
VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
return nullptr;
return clone();
}

mixxx::track::io::BeatGrid grid = m_grid;
grid.mutable_bpm()->set_bpm(bpm.value());
const mixxx::audio::FrameDiff_t beatLengthFrames = (60.0 * m_sampleRate / bpm.value());
return BeatsPointer(new BeatGrid(*this, grid, beatLengthFrames));
return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
}

} // namespace mixxx
19 changes: 15 additions & 4 deletions src/track/beatgrid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,18 +64,29 @@ class BeatGrid final : public Beats {

BeatsPointer translate(audio::FrameDiff_t offset) const override;
BeatsPointer scale(BpmScale scale) const override;
BeatsPointer setBpm(mixxx::Bpm bpm) override;
BeatsPointer setBpm(mixxx::Bpm bpm) const override;

////////////////////////////////////////////////////////////////////////////
// Hidden constructors
////////////////////////////////////////////////////////////////////////////

private:
BeatGrid(
MakeSharedTag,
audio::SampleRate sampleRate,
const QString& subVersion,
const mixxx::track::io::BeatGrid& grid,
double beatLength);
// Constructor to update the beat grid
BeatGrid(const BeatGrid& other, const mixxx::track::io::BeatGrid& grid, double beatLength);
BeatGrid(const BeatGrid& other);
BeatGrid(
MakeSharedTag,
const BeatGrid& other,
const mixxx::track::io::BeatGrid& grid,
double beatLength);
BeatGrid(
MakeSharedTag,
const BeatGrid& other);

private:
audio::FramePos firstBeatPosition() const;
mixxx::Bpm bpm() const;

Expand Down
41 changes: 22 additions & 19 deletions src/track/beatmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ class BeatMapIterator : public BeatIterator {
};

BeatMap::BeatMap(
MakeSharedTag,
audio::SampleRate sampleRate,
const QString& subVersion,
BeatList beats,
Expand All @@ -188,15 +189,18 @@ BeatMap::BeatMap(
m_beats(std::move(beats)) {
}

BeatMap::BeatMap(const BeatMap& other, BeatList beats, mixxx::Bpm nominalBpm)
: m_subVersion(other.m_subVersion),
m_sampleRate(other.m_sampleRate),
m_nominalBpm(nominalBpm),
m_beats(std::move(beats)) {
BeatMap::BeatMap(
MakeSharedTag,
const BeatMap& other,
BeatList beats,
mixxx::Bpm nominalBpm)
: BeatMap({}, other.m_sampleRate, other.m_subVersion, std::move(beats), nominalBpm) {
}

BeatMap::BeatMap(const BeatMap& other)
: BeatMap(other, other.m_beats, other.m_nominalBpm) {
BeatMap::BeatMap(
MakeSharedTag,
const BeatMap& other)
: BeatMap({}, other, other.m_beats, other.m_nominalBpm) {
}

// static
Expand All @@ -218,7 +222,7 @@ BeatsPointer BeatMap::fromByteArray(
qDebug() << "ERROR: Could not parse BeatMap from QByteArray of size"
<< byteArray.size();
}
return BeatsPointer(new BeatMap(sampleRate, subVersion, beatList, nominalBpm));
return std::make_shared<BeatMap>(MakeSharedTag{}, sampleRate, subVersion, beatList, nominalBpm);
}

// static
Expand Down Expand Up @@ -250,7 +254,7 @@ BeatsPointer BeatMap::makeBeatMap(
previousBeatPos = beatPos;
}
const auto nominalBpm = calculateNominalBpm(beatList, sampleRate);
return BeatsPointer(new BeatMap(sampleRate, subVersion, beatList, nominalBpm));
return std::make_shared<BeatMap>(MakeSharedTag{}, sampleRate, subVersion, beatList, nominalBpm);
}

QByteArray BeatMap::toByteArray() const {
Expand Down Expand Up @@ -524,9 +528,8 @@ mixxx::Bpm BeatMap::getBpmAroundPosition(audio::FramePos position, int n) const
}

BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
// Converting to frame offset
if (!isValid()) {
return BeatsPointer(new BeatMap(*this));
return clone();
}

BeatList beats = m_beats;
Expand All @@ -544,12 +547,12 @@ BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
}
}

return BeatsPointer(new BeatMap(*this, beats, m_nominalBpm));
return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, m_nominalBpm);
}

BeatsPointer BeatMap::scale(BpmScale scale) const {
if (!isValid() || m_beats.isEmpty()) {
return BeatsPointer(new BeatMap(*this));
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clone();
}

BeatList beats = m_beats;
Expand Down Expand Up @@ -588,16 +591,16 @@ BeatsPointer BeatMap::scale(BpmScale scale) const {
break;
default:
DEBUG_ASSERT(!"scale value invalid");
return BeatsPointer(new BeatMap(*this));
return clone();
}

mixxx::Bpm bpm = calculateNominalBpm(beats, m_sampleRate);
return BeatsPointer(new BeatMap(*this, beats, bpm));
return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, bpm);
}

BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) {
if (!isValid()) {
return {};
BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) const {
VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
return clone();
}

const auto firstBeatPosition = mixxx::audio::FramePos(m_beats.first().frame_position());
Expand Down
23 changes: 17 additions & 6 deletions src/track/beatmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ namespace mixxx {

class BeatMap final : public Beats {
public:

~BeatMap() override = default;

static BeatsPointer fromByteArray(
Expand Down Expand Up @@ -67,17 +66,29 @@ class BeatMap final : public Beats {

BeatsPointer translate(audio::FrameDiff_t offset) const override;
BeatsPointer scale(BpmScale scale) const override;
BeatsPointer setBpm(mixxx::Bpm bpm) override;
BeatsPointer setBpm(mixxx::Bpm bpm) const override;

private:
BeatMap(audio::SampleRate sampleRate,
////////////////////////////////////////////////////////////////////////////
// Hidden constructors
////////////////////////////////////////////////////////////////////////////

BeatMap(
MakeSharedTag,
audio::SampleRate sampleRate,
const QString& subVersion,
BeatList beats,
mixxx::Bpm nominalBpm);
// Constructor to update the beat map
BeatMap(const BeatMap& other, BeatList beats, mixxx::Bpm nominalBpm);
BeatMap(const BeatMap& other);
BeatMap(
MakeSharedTag,
const BeatMap& other,
BeatList beats,
mixxx::Bpm nominalBpm);
BeatMap(
MakeSharedTag,
const BeatMap& other);

private:
bool isValid() const override;

const QString m_subVersion;
Expand Down
24 changes: 21 additions & 3 deletions src/track/beats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
namespace mixxx {

class Beats;
typedef std::shared_ptr<Beats> BeatsPointer;
typedef std::shared_ptr<const Beats> BeatsPointer;

class BeatIterator {
public:
Expand All @@ -27,10 +27,17 @@ class BeatIterator {
/// Beats is the base class for BPM and beat management classes. It provides a
/// specification of all methods a beat-manager class must provide, as well as
/// a capability model for representing optional features.
class Beats {
///
/// All instances of this class are supposed to be managed by std::shared_ptr!
class Beats : private std::enable_shared_from_this<Beats> {
public:
virtual ~Beats() = default;

BeatsPointer clone() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term clone() is misleading here, because the Beats object is not cloned in terms of making an identical copy.
You just copy a shared pointer form a hidden weak pointer. In Qt it is called toStrongRef()
In our case maybe toBeatsPointer() or getBeatsPointer() or such.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning an immutable object is trivial be sharing its reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually this is still a clone operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused when reading it the first time. You can either add documentation that this does not really clone, or change the name. I would prefer the later.

// All instances are immutable and can be shared safely
return shared_from_this();
}

static mixxx::BeatsPointer fromByteArray(
mixxx::audio::SampleRate sampleRate,
const QString& beatsVersion,
Expand Down Expand Up @@ -166,10 +173,21 @@ class Beats {
virtual BeatsPointer scale(BpmScale scale) const = 0;

/// Adjust the beats so the global average BPM matches `bpm`.
virtual BeatsPointer setBpm(mixxx::Bpm bpm) = 0;
virtual BeatsPointer setBpm(mixxx::Bpm bpm) const = 0;

protected:
/// Type tag for making public constructors of derived classes inaccessible.
///
/// The constructors must be public for using std::make_shared().
struct MakeSharedTag {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not only used by makeShared. It is more like a "CTorProtect" or "ProtectedTag"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only needed for make_shared(). Otherwise the constructors could be hidden by making them private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Redundantly repeating the type name of the type tag class where it is not needed is useless. It must only specified explicitly when using std::make_shared(), the only purpose it was introduced for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We hit often such discussion in our reviews. I think this is sourced by our different roles.
For me as a reviewer it is important to understand the code at once without looking into other references.
You as the author are more interested to avoid redundant statements.
Please keep in mind that the human reader is not a compiler which reads thousands of lines is a second, fetched form many included files. To make the code readable I prefer redundant statements more than explaining comments, because the compiler can check them.

A plain {} as a parameter can be everything, the reader needs to look up other references to understand it. This is the same for bool function parameter by the way.

Whenever the function signature changes, a {} will still compile even though it might be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to use {} everywhere but std::make_shared() fails to deduce the type.

I don't see a reason to re-introduce the redundant type name in private forwards.


Beats() = default;

virtual bool isValid() const = 0;

private:
Beats(const Beats&) = delete;
Beats(Beats&&) = delete;
};

} // namespace mixxx