Skip to content

audiomixerboard size_t clean-up #2893

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

Jump to bottom
Merged
pljones merged 1 commit into from
Nov 16, 2022
Merged

audiomixerboard size_t clean-up #2893

pljones merged 1 commit into from
Nov 16, 2022

Conversation

@pljones
Copy link
Collaborator

@pljones pljones commented Oct 1, 2022

Short description of changes

Small patch to audiomixerboard.cpp to clean up warnings in Qt Creator. Changes int to size_t where appropriate and handles any casts required.

CHANGELOG: Refactor: use size_t for vector and array indexes that must not be negative

Context: Fixes an issue?

Fixes warnings in Qt Creator in one file. Warnings of a similar nature are also available.

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

I use it in the client I run when jamming and it seems okay.

What is missing until this pull request can be merged?

Needs a review.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

AUTOBUILD: Please build all targets

@pljones pljones added this to the Release 3.10.0 milestone Oct 1, 2022
@pljones pljones added the refactoring Non-behavioural changes, Code cleanup label Oct 1, 2022
@pljones pljones self-assigned this Oct 1, 2022
@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch 2 times, most recently from 4aa32ab to 321b705 Compare October 8, 2022 10:41
@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch 3 times, most recently from e4d9a57 to 06a05cc Compare October 13, 2022 21:14
@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch from 06a05cc to f2d0dbb Compare October 17, 2022 20:46
ann0see
ann0see reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Is there any valid reason why size_t is prefered by Qt? I doubt we get over the positive integer range anywhere.

@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch 5 times, most recently from 563f2f1 to 02c7128 Compare October 21, 2022 17:13
@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch 2 times, most recently from 09f1fc4 to 85ef075 Compare November 5, 2022 17:16
@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch from 85ef075 to fee011a Compare November 13, 2022 23:02
ann0see
ann0see approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving based on reading.

hoffie
hoffie approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

I don't see anything wrong here, but I can't say that I'm able to assess that everything is right... ;)

Fixes warnings in Qt Creator in one file. Warnings of a similar nature are also available.

What are those warnings?

@pljones
Copy link
Collaborator Author

pljones commented Nov 16, 2022

What are those warnings?

I wish cut-n-paste from the warnings was simple. They seemed to be Qt IDE rather than Compiler warnings (so the compile output didn't show them). Whilst correct (i.e. size_t can hold a larger number than int as it's unsigned), the int usage is also correct in places (we use -1 as an invalid index into an array or list, which has an index type of size_t, so doing something like

void methodName(SomeArrayType someArray) {
  int i = sizeof(someArray) - 1;
  if (i == INVALID_INDEX) {
    return;
  }
  ...
}

will get a warning, as someArray could be so big that even taking one off the size is still too large a positive number for the int to hold. In practice, we generally use no extent over about 256 (sound card buffer size is probably the worst - maybe 4K? - still well under the number an int can hold).

@pljones pljones force-pushed the patch/audiomixerboard-size_t-cleanup branch from fee011a to ce34498 Compare November 16, 2022 13:15
@pljones pljones merged commit da1e34f into jamulussoftware:master Nov 16, 2022
@pljones pljones deleted the patch/audiomixerboard-size_t-cleanup branch November 16, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hoffie hoffie hoffie approved these changes

@ann0see ann0see ann0see approved these changes

Assignees

@pljones pljones

Labels

refactoring Non-behavioural changes, Code cleanup

Projects

Tracking
Status: Done

Milestone

Release 3.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pljones @hoffie @ann0see