Added id to Clip #7617
Added id to Clip #7617
Conversation
| TrackId() : mValue(-1) {} | ||
| explicit TrackId (long value) : mValue(value) {} | ||
| TrackId() = default; | ||
| explicit TrackId (int64_t value) : mValue(value) {} | ||
|
|
||
| bool operator == (const TrackId &other) const |
There was a problem hiding this comment.
You’ve got == and <, but no !=. If someone tries to check if two TrackIds aren’t equal, they’ll be out of luck.
Add an operator!= for consistency, or if you're feeling fancy, use std::strong_ordering if you're on C++20.
There was a problem hiding this comment.
This is an AU3 class, it is designed as designed.
We do not refactor or improve AU3 classes.
We only make the minimum necessary changes to apply them to AU4
There was a problem hiding this comment.
So who and how do we decide what is considered minimum?
There was a problem hiding this comment.
the author of the changes is obvious. He knows what he needs to change minimally in AU3 to achieve his goal. If he didn't do something, then it's probably not necessary to achieve the goal.
| TrackId() : mValue(-1) {} | ||
| explicit TrackId (long value) : mValue(value) {} | ||
| TrackId() = default; | ||
| explicit TrackId (int64_t value) : mValue(value) {} |
There was a problem hiding this comment.
Nice!! (good use of explicit)
| private: | ||
| long mValue; | ||
| int64_t mValue = -1; |
There was a problem hiding this comment.
-1 is used here as a "special" value, but this can be risky if the rest of the code doesn’t expect it. Plus, there's no guarantee that TrackId is always initialised properly if someone bypasses the constructors.
Why not use std::optional<int64_t> for mValue instead to make it clear when the ID is valid or not, or make sure that the class can’t be misused without proper initialisation.
There was a problem hiding this comment.
| private: | ||
| long mValue; | ||
| int64_t mValue = -1; |
There was a problem hiding this comment.
Stop using -1 as a “magic” value to represent an uninitialized state. This can be confusing and easily misinterpreted as a valid ID. Instead, use std::optional<int64_t>, which explicitly shows whether the ID is set.
There was a problem hiding this comment.
Tell us more here, usually they don't use optinal for ID because they need performance, very often they search by ID, for example... What is your experience using optinal in such cases?
| TrackId() : mValue(-1) {} | ||
| explicit TrackId (long value) : mValue(value) {} | ||
| TrackId() = default; | ||
| explicit TrackId (int64_t value) : mValue(value) {} | ||
|
|
||
| bool operator == (const TrackId &other) const |
There was a problem hiding this comment.
Add the missing comparison operators. Defining and < leaves TrackId with incomplete comparisons.
There was a problem hiding this comment.
| @@ -220,10 +220,14 @@ double WaveClipChannel::GetStretchRatio() const | |||
| return GetClip().GetStretchRatio(); | |||
| } | |||
|
|
|||
|
|
|||
| static int64_t s_lastClipId = 0; | |||
|
|
|||
| WaveClip::WaveClip(size_t width, | |||
There was a problem hiding this comment.
WaveClip::WaveClip(size_t width, const SampleBlockFactoryPtr &factory, sampleFormat format, int rate)
: mId(++s_lastClipId), mRate(rate), mSequences(width) {
assert(width > 0);
}
In the constructor, initialize member variables directly in the initializer list rather than in the body. For instance, mId, mRate, and mSequences.resize(width) should ideally be in the initializer list:
There was a problem hiding this comment.
made in accordance with the current AU3 style
| @@ -27,10 +26,10 @@ Au3WaveTrack* DomAccessor::findWaveTrack(Au3Project& prj, const Au3TrackId& au3t | |||
| return dynamic_cast<Au3WaveTrack*>(findTrack(prj, au3trackId)); | |||
| } | |||
|
|
|||
| std::shared_ptr<Au3WaveClip> DomAccessor::findWaveClip(Au3WaveTrack* track, uint64_t au3ClipId) | |||
| std::shared_ptr<Au3WaveClip> DomAccessor::findWaveClip(Au3WaveTrack* track, int64_t au3ClipId) | |||
| { | |||
| for (const std::shared_ptr<Au3WaveClip>& interval : track->Intervals()) { | |||
There was a problem hiding this comment.
Using auto shortens things up and still keeps everything clear, especially when the types get a bit lengthy.
Also the type here does not bring and benefit as you are not using the type...
There was a problem hiding this comment.
why do you think it's better to use auto? it's not clear what type we're dealing with when we review or read the code
|
I like that and I know that we wanted to do this thing from a long time but we simply couldn't, but: |
c558e5a
to
68e96ba
Compare
No description provided.