Fixes style on "SampleBuffer.cpp" and "SampleBuffer.h"

[IanCaio]

The code style on the files src/core/SampleBuffer.cpp and include/SampleBuffer.h was fixed. Most of the fixes were indentation, spacing, variable names (dropping the underscore on arguments and using camelCase) and changes to improve readability. A list of the affected methods is at the end of the PR message.

The bugfix mentioned below was extracted to a separate PR (#5796) which is already merged:

One bug on SampleBuffer::decodeSampleSF was fixed: A variable (previously named sf_rr) was declared as a boolean. But it was used to store the return value of libsnd's method sf_read_float, which returns a number of type sf_count_t with the number of frames read. It's later used in a comparison against the expected number of frames read to check for errors. Problem is since it's a boolean, any number of frames read except 0 will be converted to true. So we will later have the following comparison: if( sf_rr < sf_info.channels * frames ). I think according to the C++ standards, sf_rr will be converted to a numeric value of 1 or 0, so most of the times this comparison will return true, even when we have no errors on our read call. This was probably not noticed because the method doesn't do anything in case of errors besides triggering a debug message if the build was of type DEBUG. Bug was fixed by declaring the variable with the appropriate type. It was also renamed for more clarity (to sfFramesRead).

On SampleBuffer::decodeSampleOGGVorbis() I removed the comment block of one of the printfs that warns the user of an error value being returned on the vorbis library read method. It seems to be triggered when loading certain types of files even though they load fine (I noticed it on .ds files), so I can comment it out again. I'm not sure that's an appropriate fix though, maybe hiding them behind the DEBUG macro would be better.

As agreed upon, #5765 should be merged before this PR to avoid the merge conflicts on his end. Also, these changes shouldn't cause any issues to classes that use SampleBuffer since they were only internal, no method names or parameters list were changed.

Methods that were affected:

Method	Reviewed?
SampleBuffer constructors	❌
SampleBuffer destructor	❌
SampleBuffer::sampleRateChanged()	❌
SampleBuffer::update()	❌
SampleBuffer::convertIntToFloat()	❌
SampleBuffer::directFloatWrite()	❌
SampleBuffer::normalizeSampleRate()	❌
SampleBuffer::resample()	❌
SampleBuffer::handleState::handleState()	❌
SampleBuffer::handleState::~handleState()	❌
SampleBuffer::decodeSampleSF()	❌
qfileReadCallback()	❌
qfileSeekCallback()	❌
qfileCloseCallback()	❌
qfileTellCallback()	❌
SampleBuffer::decodeSampleOGGVorbis() => Uncommented code	❌
SampleBuffer::decodeSampleDS()	❌
SampleBuffer::getLoopedIndex()	❌
SampleBuffer::getPingPongIndex()	❌
SampleBuffer::getSampleFragment()	❌
SampleBuffer::play()	❌
SampleBuffer::visualize()	❌
SampleBuffer::openAudioFile()	❌
SampleBuffer::openAndSetAudioFile()	❌
SampleBuffer::openAndSetWaveformFile()	❌
flacStreamEncoderWriteCallback()	❌
flacStreamEncoderMetadataCallback()	❌
SampleBuffer::toBase64()	❌
SampleBuffer::setAudioFile()	❌
struct flacStremDecoderClientData	❌
flacStreamDecoderReadCallback()	❌
flacStreamDecoderWriteCallback()	❌
flacStreamDecoderMetadataCallback()	❌
flacStreamDecoderErrorCallback()	❌
SampleBuffer::loadFromBase64()	❌
SampleBuffer::setStartFrame()	❌
SampleBuffer::setEndFrame()	❌
SampleBuffer::setAmplification()	❌
SampleBuffer::setReversed()	❌
SampleBuffer::setLoopStartFrame()	❌
SampleBuffer::setLoopEndFrame()	❌
SampleBuffer::setAllPointFrames()	❌
SampleBuffer::sampleLength()	❌
SampleBuffer::setFrequency()	❌
SampleBuffer::setSampleRate()	❌
SampleBuffer::userWaveSample()	❌

Some issues that this PR doesn't fix (TODO):

• Rename sampleFrame to SampleFrame: Spans several files.
• Rename handleState to HandleState: Spans fewer files, but is used outside SampleBuffer.cpp.
• Remove buf and fbuf declarations from SampleBuffer::update and let them be declared on the decode methods instead.
• Remove call to qMin(frames, loopEnd - index); because there was a previous conditional with if (index + frames <= loopEnd) ... return;, meaning that frames will be smaller than loopEnd - index. (Need to double check. Might be safer to keep the call for the long-term code maintenance, just bringing it up)
• Change SampleBuffer::getSampleFragment to use a reference to a pointer instead of a pointer to a pointer.
• Decouple GUI method from core (SampleBuffer::visualize).

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.48%2Bgfaebb86-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11341-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.48%2Bgfaebb86-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11341?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11342-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11342?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11344-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11344?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/xtbbhmp17xfegwmy/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36679436"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/k493imr45bpdg5mv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36679436"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://11343-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.48%2Bgfaebb86e0-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11343?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "553e2ca502a75ba7245076ee0aad31d1be629d8c"}

[Spekular]

I'd kind of prefer to get #5524 and #5727 merged before this 🙃

[IanCaio]

@Spekular No problem! They just add methods to SampleBuffer.cpp so solving the conflicts will be just using the refactored file and adding the move/copy constructors.

From what I understood #5727 repeats some changes already made at #5524 right and adds the constructors for SampleTCO?

[Spekular]

Yeah, #5727 is currently just some changes extracted from #5524. There are things to fix with it before it can be merged though, and then I'll try to rebase #5524 so the diff is smaller and more focused on the knife-specific changes.

[PhysSong]

Could you split this to fixes and style changes so we can merge fixes first?

[IanCaio]

Sure, I'll do it later today!

[IanCaio]

Merged master and fixed conflict with changes from PR #5765

[JohannesLorenz]

@IanCaio I can review this.

There are currently conflicts with master. Could you please do a rebase in your local repo to the current master and force-push it over this PR?

[IanCaio]

@IanCaio I can review this.

There are currently conflicts with master. Could you please do a rebase in your local repo to the current master and force-push it over this PR?

Thanks! The conflicts aren't showing here on Github (there was one fixed on the last commit) but I'll merge master, fix any that show and push soon. I'm just trying to address a review on another PR first.

[JohannesLorenz]

@IanCaio Still conflicting files 😄

[IanCaio]

@IanCaio Still conflicting files smile

Oh, that was a new one from #5816 , just fixed it!

[JohannesLorenz]

I'm just writing down my own strategy to check that this PR is valid:

1. I ran clang-format in both new and old code to get rid of whitespace/linebreaks in the diff
2. I replaced NULL by nullptr in both versions.
3. Then, I ended up with 2 files old.cpp and new.cpp which almost only differ by variable renames. There are two dangers with variable renames:
   1. A variable was _x in old.cpp and is now changed to x in new.cpp by this PR. However, in the same function, another (e.g. class member/global) variable x was already used in old.cpp, which is now hidden by the new x. I.e. another variable is now being used; this is a logic error. In this case, we have to check that for every rename _x -> x there is no x in the old version. So grep for x in the old version.
   2. A variable was _x in old.cpp and is now still _x in new.cpp, because the PR author forgot to rename this occurrence of the variable. The code may still compile and now use a (e.g. class member/global) _x, which has previously been hidden by the local _x parameter. This would mean a different variable is now used, e.g. a logic error. However, all replaced variables started with _, and a grep '[^a-zA-Z0-9_]_' -r new.cpp succesfully showed that no variable has been forgotten.

Can anyone check whether my strategy would be correct, and complete?

[IanCaio]

I'm just writing down my own strategy to check that this PR is valid:

1. I ran clang-format in both new and old code to get rid of whitespace/linebreaks in the diff

2. I replaced NULL by nullptr in both versions.

3. Then, I ended up with 2 files old.cpp and new.cpp which almost only differ by variable renames. There are two dangers with variable renames:

4. A variable was _x in old.cpp and is now changed to x in new.cpp by this PR. However, in the same function, another (e.g. class member/global) variable x was already used in old.cpp, which is now hidden by the new x. I.e. another variable is now being used; this is a logic error. In this case, we have to check that for every rename _x -> x there is no x in the old version. So grep for x in the old version.

5. A variable was _x in old.cpp and is now still _x in new.cpp, because the PR author forgot to rename this occurrence of the variable. The code may still compile and now use a (e.g. class member/global) _x, which has previously been hidden by the local _x parameter. This would mean a different variable is now used, e.g. a logic error. However, all replaced variables started with _, and a grep '[^a-zA-Z0-9_]_' -r new.cpp succesfully showed that no variable has been forgotten.

Can anyone check whether my strategy would be correct, and complete?

I think the per-method approach would be the most fail-proof, but from what I read the logic is fine and covers it all! As far as I remember, most name substitutions didn't cause conflict. decodeSampleSF and decodeSampleOGGVorbis were the only ones that required name changes to avoid conflict if I'm not mistaken (parameter was _f but there was a local f variable, so I changed _f to 'fileName`).

[JohannesLorenz]

I'm through all formatting changes and variable renames. I'll only do a few checks for other things.

@IanCaio when all is OK, shall I squash & merge this?

[JohannesLorenz]

Review finished. Only 1 minor comment.

[IanCaio]

I'm through all formatting changes and variable renames. I'll only do a few checks for other things.

@IanCaio when all is OK, shall I squash & merge this?

Sure! When you think it's good you can merge it

[JohannesLorenz]

Merged. Thanks for the contribution!