-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Removed raw array pointer types for std::vector #4341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this cleanup.
src/analyzer/analyzergain.cpp
Outdated
delete[] m_pRightTempBuffer; | ||
m_pLeftTempBuffer = new CSAMPLE[halfLength]; | ||
m_pRightTempBuffer = new CSAMPLE[halfLength]; | ||
m_pLeftTempBuffer = std::vector<CSAMPLE>(halfLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using resize() is sufficient here and better reveals the intention.
m_outputL = new float[MAX_BUFFER_LEN]; | ||
m_outputR = new float[MAX_BUFFER_LEN]; | ||
m_params = new float[pManifest->parameters().size()]; | ||
m_inputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assignments should be replaced by direct initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand your suggestion but could you help me understand your term "replaced by direct initialization"?
Do you mean to express, "replaced by using the constructor's member initialisation lists (directly)"? Currently, the objects are being assigned via "direct initialization" using your term in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add m_inputL{MAX_BUFFER_LEN}
to the initializer list instead of default constructing and then re-assigning it.
src/effects/lv2/lv2manifest.cpp
Outdated
m_default = new float[numPorts]; | ||
lilv_plugin_get_port_ranges_float(m_pLV2plugin, m_minimum, m_maximum, | ||
m_default); | ||
m_minimum = std::vector<float>(numPorts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, direct initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/effects/lv2/lv2manifest.cpp
Outdated
@@ -153,9 +153,6 @@ LV2Manifest::LV2Manifest(const LilvPlugin* plug, | |||
} | |||
|
|||
LV2Manifest::~LV2Manifest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please either delete empty destructors or replace them with ...() override = default;
in derived classes. If needed move the = default;
part into the .cpp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule of Zero is preferred as it is less code to maintain. Happy to remove all empty destructors as first choice. 😸
If the = default
is inline, shouldn't be rather in the header declaration? (Remember, = default
nearly always adds a hidden noexcept
to its declaration.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If inlining the destructors is possible than yes.
Destructors should never throw in the first place. Fallible shutdown procedures have to be implemented outside of destructors as separate functions.
src/encoder/encoderfdkaac.cpp
Outdated
@@ -283,7 +279,7 @@ int EncoderFdkAac::initEncoder(mixxx::audio::SampleRate sampleRate, QString* pUs | |||
// This initializes the encoder handle but not the encoder itself. | |||
// Actual encoder init is done below. | |||
aacEncOpen(&m_aacEnc, 0, m_channels); | |||
m_pAacDataBuffer = new unsigned char[kOutBufferBits * m_channels](); | |||
m_pAacDataBuffer = std::vector<unsigned char>(kOutBufferBits * m_channels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use resize() to avoid repeating the type?
src/util/circularbuffer.h
Outdated
@@ -14,16 +15,14 @@ class CircularBuffer { | |||
public: | |||
CircularBuffer(unsigned int iLength) | |||
: m_iLength(iLength), | |||
m_pBuffer(new T[m_iLength]), | |||
m_pBuffer(std::vector<T>(m_iLength)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_pBuffer{m_iLength}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both m_pBuffer(m_iLength)
and m_pBuffer{m_iLength}
work for me. Did you try it? Please be more specific.
Comment also applies to all other occurrences.
src/util/fifo.h
Outdated
@@ -9,18 +9,16 @@ template <class DataType> | |||
class FIFO { | |||
public: | |||
explicit FIFO(int size) | |||
: m_data(NULL) { | |||
: m_data{std::vector<DataType>(size)} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_data{size}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/util/circularbuffer.h
Outdated
@@ -14,16 +15,14 @@ class CircularBuffer { | |||
public: | |||
CircularBuffer(unsigned int iLength) | |||
: m_iLength(iLength), | |||
m_pBuffer(new T[m_iLength]), | |||
m_pBuffer(std::vector<T>(m_iLength)), | |||
m_iWritePos(0), | |||
m_iReadPos(0) { | |||
// No need to clear the buffer because we consider it to be empty right | |||
// now. | |||
} | |||
|
|||
virtual ~CircularBuffer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual ~CircularBuffer() = default;
src/util/rotary.cpp
Outdated
m_pFilter = new double[m_iFilterLength]; | ||
: m_iFilterLength(kiRotaryFilterMaxLen), | ||
m_iFilterPos(0), | ||
m_pFilter(std::vector<double>(m_iFilterLength)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_pFilter{m_iFilterLength}
src/util/rotary.cpp
Outdated
m_pFilter(std::vector<double>(m_iFilterLength)), | ||
m_dCalibration(1.0), | ||
m_dLastValue(0.0), | ||
m_iCalibrationCount(0) { | ||
for (int i = 0; i < m_iFilterLength; ++i) { | ||
m_pFilter[i] = 0.; | ||
} | ||
} | ||
|
||
Rotary::~Rotary() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete the obsolete destructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing from bare arrays to std::vector comes with a penalty on memory consumption, because vector allocates more elements than required so it doesn't need to reallocate immediately on insertion.
m_inputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN); | ||
m_inputR = std::vector<CSAMPLE>(MAX_BUFFER_LEN); | ||
m_outputL = std::vector<CSAMPLE>(MAX_BUFFER_LEN); | ||
m_outputR = std::vector<CSAMPLE>(MAX_BUFFER_LEN); | ||
m_params = std::vector<float>(pManifest->parameters().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::array
is probably the better choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know actually. If we rely on the fact that we don't reallocate later, we should use std::array. If we rather want to save some memory, std::vector might be the better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the size is known at compile time then we could use std::array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this particular case, its known because its a #define
. However, I don't know if thats because we actually know that there is only ever going to be exactly MAX_BUFFER_LEN
elements, or if we just allocate this many elements statically to make sure we don't have to reallocate in RT code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAX_BUFFER_LEN is an ugly quick hack. The buffers should be allocated based on the buffer size configured in the preferences and reallocated when that is changed. That is outside of the scope of this PR, but for this PR we should not rely on the buffer size being known at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well its standard practice in microcontroller-realm where there is no heap-allocation. I assume this code was written by daniel which is why he has less issues with this method than we have.
src/encoder/encoderfdkaac.cpp
Outdated
outputBuf.bufs = (void**)&m_pAacDataBuffer; | ||
outputBuf.bufs = reinterpret_cast<void**>(m_pAacDataBuffer.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure whether this is legal, but I'd need someone else to have a look at it. Would static_cast
also work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! That's why reinterpret_cast is dangerous. The &
got lost!
outputBuf.bufs = &static_cast<void*>(m_pAacDataBuffer.data());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So note, .bufs
datatype is void**
. This means one is already in the "danger zone" 👀 !
static_cast<T>
, (&static_cast<T>
) only works for datatypes it can "safely" convert to.
With void**
it is not possible to assume "safety". So static_cast<T>
cannot work in this realm. You need to use reinterpret_cast<void**>
to make "the magic" happen. ✨ 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevertheless, m_pAacDataBuffer.data()
is the wrong pointer. The API expects an array of pointers to multiple buffers and not just a pointer to a single buffer, i.e. double indirection!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. So unsigned char* m_pAacDataBuffer;
and .bufs = (void**)&m_pAacDataBuffer;
was how it was declared and assigned previously.
In your opinion which one would be preferable, could you help me decide?
// [1]
.bufs = reinterpret_cast<void**>(std::addressof(*std::begin(m_pAacDataBuffer)));
// [2]
.bufs = reinterpret_cast<void**>(std::addressof(*m_pAacDataBuffer.data()));
// [3]
.bufs = reinterpret_cast<void**>(&(*m_pAacDataBuffer.data()));
// [4]
.bufs = reinterpret_cast<void**>(std::addressof(std::begin(m_pAacDataBuffer)[0]));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this safe variant that does not require any explicit casts:
void* outputBufs[] = {m_pAacDataBuffer.data()};
outputBuf.bufs = outputBufs;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, a double implicit way seems more elegant to solve this one. Will go with this. 😺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version with the temporary array also reveals the actual semantics.
src/util/fifo.h
Outdated
: m_data{std::vector<DataType>(size)} { | ||
size = roundUpToPowerOf2(size); | ||
// If we can't represent the next higher power of 2 then bail. | ||
if (size < 0) { | ||
return; | ||
} | ||
m_data = new DataType[size]; | ||
PaUtil_InitializeRingBuffer( | ||
&m_ringBuffer, sizeof(DataType), size, m_data); | ||
&m_ringBuffer, sizeof(DataType), size, m_data.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PA_RingBuffer and the vector now have differing sizes which some other code might rely upon.
src/util/rotary.cpp
Outdated
m_pFilter(std::vector<double>(m_iFilterLength)), | ||
m_dCalibration(1.0), | ||
m_dLastValue(0.0), | ||
m_iCalibrationCount(0) { | ||
for (int i = 0; i < m_iFilterLength; ++i) { | ||
m_pFilter[i] = 0.; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_pFilter(std::vector<double>(m_iFilterLength)), | |
m_dCalibration(1.0), | |
m_dLastValue(0.0), | |
m_iCalibrationCount(0) { | |
for (int i = 0; i < m_iFilterLength; ++i) { | |
m_pFilter[i] = 0.; | |
} | |
m_pFilter(std::vector<double>(m_iFilterLength, 0.0)), | |
m_dCalibration(1.0), | |
m_dLastValue(0.0), | |
m_iCalibrationCount(0) { |
we can get rid of the manual initialization loop if we rely on the constructor to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_pFilter{m_iFilterLength, 0.0}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_imageData = new uchar[size * size * 4]; | ||
m_qImage = QImage(m_imageData, size, size, 0, QImage::Format_ARGB32); | ||
m_imageData = std::vector<unsigned char>(size * size * 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uchar
is a Qt typedef which is also used by their API. While its technically just an unsigned char
, they could decide to change this in the future which would then break our code. Therefore I'd stick to using uchar
.
if (m_imageData != nullptr) { | ||
memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4); | ||
} | ||
m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4); | |
m_imageData.clear() |
just empty the vector, doesn't reallocate but leaves the capacity unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's wrong. You are only allowed to access the elements up to the (initialized) size of the vector. The elements between size and capacity must not be used. Clearing the vector would set its size to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, then this should be valid:
m_imageData = std::vector<unsigned char>(m_iSize * m_iSize * 4); | |
m_imageData.clear() | |
m_imageData.resize(m_iSize * m_iSize * 4); |
.clear() sets the size to 0 and then .resize defaults constructs all the elements. heap-allocation and deallocation is still avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear() is redundant, just resize() with initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading cppreference correctly, when the new size is smaller than the current size, the vector just gets truncated, if its larger, only then will the space be initialized with the elements. So when the new size is equal to the old size, the resize operation is a no-op and will leave the old items in the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you are right! If re-initialization of all elements is required then resize() alone doesn't work as expected.
But then we should better wrap consecutive clear() + resize() calls into a helper function and document it properly. If it is only used once an inline comment is sufficient.
Thanks for all the feedback / comments, and joint agreement to handle raw dynamic elements with more automatic datatypes. Will be adding your suggestion very soon. Some notes:
|
Thank you. Plain C arrays are a dangerous, confusing mess. |
@tcoyvwac are you on Zulip? Since you seem to like doing code cleanup, it would be a really big help if you work on fixing compile errors with Qt6. You can work on that by configuring CMake with |
7964ada
to
0da929d
Compare
When all the PR comments are basically answered / resolved, will rebase all the commits into one so it can be ready to be merged. For now, just making a separate commit for edits so it is easier to read the PR. |
0da929d
to
dcb28bd
Compare
src/encoder/encoderfdkaac.cpp
Outdated
@@ -382,12 +378,12 @@ void EncoderFdkAac::encodeBuffer(const CSAMPLE* samples, const int sampleCount) | |||
} | |||
|
|||
void EncoderFdkAac::processFIFO() { | |||
if (!m_pInputFifo || !m_pFifoChunkBuffer) { | |||
if (!m_pInputFifo || !m_pFifoChunkBuffer.data()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_pFifoChunkBuffer.empty()?
src/encoder/encoderfdkaac.cpp
Outdated
@@ -414,7 +411,8 @@ void EncoderFdkAac::processFIFO() { | |||
// Output (result) Buffer | |||
AACENC_BufDesc outputBuf; | |||
outputBuf.numBufs = 1; | |||
outputBuf.bufs = (void**)&m_pAacDataBuffer; | |||
void* dataBufs[] = {m_pAacDataBuffer.data()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunkBuffer vs. dataBufs? Please try to keep the naming consistent.
src/test/analyserwaveformtest.cpp
Outdated
@@ -19,8 +20,8 @@ class AnalyzerWaveformTest : public MixxxTest { | |||
protected: | |||
AnalyzerWaveformTest() | |||
: aw(config(), QSqlDatabase()), | |||
bigbuf(nullptr), | |||
canaryBigBuf(nullptr) { | |||
bigbuf(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No explicit default constructor calls required.
src/util/circularbuffer.h
Outdated
@@ -14,16 +15,14 @@ class CircularBuffer { | |||
public: | |||
CircularBuffer(unsigned int iLength) | |||
: m_iLength(iLength), | |||
m_pBuffer(new T[m_iLength]), | |||
m_pBuffer(std::vector<T>(m_iLength)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both m_pBuffer(m_iLength)
and m_pBuffer{m_iLength}
work for me. Did you try it? Please be more specific.
Comment also applies to all other occurrences.
62c0fe6
to
8f89ba9
Compare
src/util/fifo.h
Outdated
@@ -9,18 +9,16 @@ template <class DataType> | |||
class FIFO { | |||
public: | |||
explicit FIFO(int size) | |||
: m_data(NULL) { | |||
: m_data(size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of m_data
now differs from size
. size
has to be adjusted before allocating the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 💭 One option is a
.resize()
wherem_data
is assigned to instead. - 💭 Another option would be to write
: m_data(roundUpToPowerOf2(size))
to keep constructor initialisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize it with the correct size and then use m_data.size() instead of size
if (m_imageData != nullptr) { | ||
memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4); | ||
} | ||
m_imageData.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment why clear() is needed before resize()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't needed. Not sure why I added it! Will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required, see #4341 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is inside VinylControlSignalWidget::resetWidget()
, would be happy to replace the code in the function with m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4);
(or explicit datatype form: std::vector<unsigned char>(m_iSize * m_iSize * 4);)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that the std::vector
's internal datatype should be uchar
as @Swiftb0y wrote in another comment. Looking at the git-blame for vinylcontrolsignalwidget.h
, the previous array was declared as unsigned char
. But agreeing with SwiftB0y, will update the std::vector
's internal datatype to uchar
.
The decltype
form is nice because there is need to worry about these stale (datatype) declarations...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the resize really needed? According to the legacy code std::fill()
is sufficient here. We don't even need to repeat the size calculation which is a welcome side-effect because it reduces redundancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resonate with your questioning around the code @uklotzde. I agree that std::fill
would be a good candidate.
This changeset / The PR is just to swiftly wrap raw array pointer objects / types around more "automatic" datatypes (and avoid the purpose of one's datatype's code).
After the objects are more "secure", the "implementation" these objects / types exist in, become more visible to additional changes, for example: std::fill
.
m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4);
(or .clear()
& .resize(m_iSize * m_iSize * 4)
) is normally~ (reasonable confidence) a signature for a std::fill
replacement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we should defer this change until later? memset
produces the same outcome as std::fill
. You could even preserve the invocation of memset
, but the code would be less readable than with std::fill
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no objections to adding this. 😸
Until explicit permission has been granted to expand the PR to cover things like std::fill
, I am trying to make the changeset very conservatively match the current codebase as close and as small of an edited footprint as possible, because it is polite PR etiquette to the Mixxx project.
Happy to add the std::fill
changes if you like of course. I would like to merge the PR sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::fill
has now been added to the changeset / PR. ✔️
Please fix the merge conflict from #4386. |
8f89ba9
to
e0a123b
Compare
pIn, | ||
halfLength); | ||
SampleUtil::applyGain(m_pLeftTempBuffer.data(), 32767, halfLength); | ||
SampleUtil::applyGain(m_pRightTempBuffer.data(), 32767, halfLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, it would be great to change the SampleUtil functions to not take plain C arrays as parameters.
21748f3
to
3dfdf29
Compare
Are there any more changes needed / requested? |
if (m_imageData != nullptr) { | ||
memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4); | ||
} | ||
m_imageData = decltype(m_imageData)(m_iSize * m_iSize * 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear() followed by resize() would not reallocate if not necessary. Did you consider this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(VinylControlSignalWidget::setSize()
exists, so the function resetWidget()
is confusing). A std::fill
was the first choice because a "resize" is not being done in the function, just a setting of the value 0
.
Because maybe adding std::fill
to this changeset could be out of scope, your suggestion clears things up ✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment below. Let's don't change the behavior of the code by simply using std::fill()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know std::fill
was a thing, seems like the perfect tool here.
3dfdf29
to
9468ee5
Compare
if (m_imageData != nullptr) { | ||
memset(m_imageData, 0, sizeof(uchar) * m_iSize * m_iSize * 4); | ||
} | ||
m_imageData.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the resize really needed? According to the legacy code std::fill()
is sufficient here. We don't even need to repeat the size calculation which is a welcome side-effect because it reduces redundancy.
Notes: * m_pFifoChunkBuffer was actualy an array pointer; was not deleted safely. Refactor fixes memory leak. * Used void*[] implicit conversions for protobuf. * Used "auto _ = std::vector{}" style for some assignments. * Default'ed empty virtual destructors and fully removed other empty destructors from codebase. * Used std::vector<T>.resize() in place of raw-new.
9468ee5
to
242e631
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank your for all the efforts and discussions! I guess this is ready now. LGTM
If anyone else wants to take a final look please do and then merge. Otherwise I will merge it later.
Thank you very much for your understanding and helpful advice everyone! |
Will also be fixed by mixxxdj#4341.
The changeset's style tries to match like-for-like the current code(base).
Note: m_pFifoChunkBuffer was actually an array pointer; was not deleted safely. Refactor fixes memory leak.
An exception was made for pBuffer in util/sample.cpp. Refactoring will be done later.