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.

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

Define fpp_t and f_cnt_t to be of size_t #7363

Merged
merged 22 commits into from
Jul 9, 2024

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Jul 2, 2024

Defines fpp_t and f_cnt_t to be of size_t. Most of the codebase used fpp_t to represent the size of an array of sample frames, which is incorrect, given that fpp_t was only 16 bits when sizes greater than 32767 are very possible.

There were discussions about using std::size_t, but I left it as size_t so that it can be adopted a bit easier in the code, and not end up in a situation where we mix up std::size_t and size_t.

@sakertooth sakertooth changed the title Replace use fpp_t and f_cnt_t with size_t Replace use of fpp_t and f_cnt_t with size_t Jul 2, 2024
Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

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

A few minor suggestions. The style changes seem to be too much so feel free to just resolve what I pointed out and neglect the rest

Also, you added mingw-std-threads back in. Minor oversight.

include/AudioDevice.h Outdated Show resolved Hide resolved
include/AudioEngine.h Outdated Show resolved Hide resolved
include/AudioEngineProfiler.h Outdated Show resolved Hide resolved
include/AudioDevice.h Outdated Show resolved Hide resolved
include/AudioFileMP3.h Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessor.cpp Outdated Show resolved Hide resolved
plugins/MultitapEcho/MultitapEcho.cpp Outdated Show resolved Hide resolved
@sakertooth
Copy link
Contributor Author

Also, you added mingw-std-threads back in. Minor oversight.

Not an oversight, I just don't know how to fix it. This branch does not have mingw-std-threads, surely. It is up to date with master already.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 3, 2024

Delete the submodule folder manually and commit

@sakertooth
Copy link
Contributor Author

Oh nevermind you're totally right, I added it in somehow 🤔.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 3, 2024

That seems to be the reason for ci fail too

@sakertooth
Copy link
Contributor Author

Also, I'm not going to worry about style for this PR. For these kind of PRs that span across many files, its far too time consuming to format each of the changes.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 3, 2024

Also, I'm not going to worry about style for this PR.

Alright then

@DomClark
Copy link
Member

DomClark commented Jul 3, 2024

There were discussions about using std::size_t, but I left it as size_t so that it can be adopted a bit easier in the code, and not end up in a situation where we mix up std::size_t and size_t.

They're the same type, just a different name, so there's no concern about mixing them up. I'm also not sure how using plain size_t makes it easier to adopt (besides being five characters shorter, but programming isn't about minimising the character count) - all it does is give us something we need to change yet again in the future, which sounds like more work to me.

A smaller change, simply redefining fpp_t and f_cnt_t would have been fine too, given that we want to replace most of these with std::span in the future.

@sakertooth
Copy link
Contributor Author

sakertooth commented Jul 3, 2024

They're the same type, just a different name, so there's no concern about mixing them up. I'm also not sure how using plain size_t makes it easier to adopt (besides being five characters shorter, but programming isn't about minimising the character count) - all it does is give us something we need to change yet again in the future, which sounds like more work to me.

I figured that developers currently are more familiar with specifying size_t rather than std::size_t, and catching the use of size_t instead of std::size_t in each new PR might be a lot of work. In that case, inevitably, the code has a high chance of having both size_t and std::size_t, and I like it more when a codebase is consistent in its choices and how its being read.

A smaller change, simply redefining fpp_t and f_cnt_t would have been fine too, given that we want to replace most of these with std::span in the future.

Yeah, this is easier to do. I will do that instead and define them as std::size_t (size_t). 👍

@sakertooth sakertooth changed the title Replace use of fpp_t and f_cnt_t with size_t Define fpp_t and f_cnt_t to be of size_t Jul 3, 2024
@sakertooth
Copy link
Contributor Author

A smaller change, simply redefining fpp_t and f_cnt_t would have been fine too, given that we want to replace most of these with std::span in the future.

I just realized we already have some code now that uses size_t instead of fpp_t or f_cnt_t for frame count, so replacing it throughout the codebase might actually be more ideal for consistency and readability, and then more ideal obviously would be to use std::span later. Since this PR has some specific changes to make the code compile though, replacing it throughout the codebase here would mask those specific changes as it would blow up the diff, which should be reviewed if possible.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 3, 2024

You still didn't remove the submodule 😅

@sakertooth
Copy link
Contributor Author

You still didn't remove the submodule 😅

I did. It's not here anymore.

@sakertooth
Copy link
Contributor Author

Also, I'm not going to worry about style for this PR. For these kind of PRs that span across many files, its far too time consuming to format each of the changes.

Nevermind, since I'm not worrying about replacing fpp_t and f_cnt_t with size_t across the codebase, it's fine to consider style. These variables can slowly be phased out over time in other PRs.

src/core/Instrument.cpp Outdated Show resolved Hide resolved
plugins/Vibed/VibratingString.cpp Outdated Show resolved Hide resolved
@sakertooth sakertooth merged commit 1420a1f into LMMS:master Jul 9, 2024
10 checks passed
@sakertooth sakertooth deleted the replace-fpp_t-size_t branch July 9, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants