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

Fix overflow warnings from loop index #3161 #3164

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

softins
Copy link
Member

@softins softins commented Aug 29, 2023

Short description of changes

Intended to resolve CodeQL messages "Multiplication result converted to larger type".

CHANGELOG: Client: (Refactor) Prevent multiplication result converting to larger type

Context: Fixes an issue?

Fixes: #3161
This fix is an alternative to the PR #3162, and arguably slightly more efficient. It avoids the use of casts.

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

No

Status of this Pull Request

Looks like it fixes the failed checks:
https://github.com/jamulussoftware/jamulus/security/code-scanning?query=pr%3A3164

(Before: https://github.com/jamulussoftware/jamulus/security/code-scanning?query=branch%3Amain)

What is missing until this pull request can be merged?

Nothing. Ready to go.

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

@softins softins requested a review from pljones August 29, 2023 20:12
@ann0see ann0see added the refactoring Non-behavioural changes, Code cleanup label Aug 29, 2023
@softins
Copy link
Member Author

softins commented Aug 30, 2023

Runs fine on my pi. I know #3162 (which this replaces) was tagged with the 3.11.0 milestone, but is there any reason this PR couldn't go straight in now?

@pljones
Copy link
Collaborator

pljones commented Aug 30, 2023

image
Any idea what's caused this in the CodeQL check?

@pljones
Copy link
Collaborator

pljones commented Aug 30, 2023

image
However, the "11" rather than the "14" indicates success.

@pljones
Copy link
Collaborator

pljones commented Aug 30, 2023

I'm happy for it to go into 3.10.0 as it's not going to affect translations or behaviour.

@softins
Copy link
Member Author

softins commented Aug 30, 2023

image Any idea what's caused this in the CodeQL check?

No, I was going to ask about it. I'll do a bit of digging.

@softins softins requested a review from ann0see August 30, 2023 11:42
@softins
Copy link
Member Author

softins commented Aug 30, 2023

I think it's something to do with the autobuild efficiency changes introduced by @hoffie in 51830196b, but I don't really understand what's happening there.

@pljones
Copy link
Collaborator

pljones commented Aug 30, 2023

OK, it's probably not something this issue/PR needs to worry about. Maybe raise a new issue?

@ann0see Are you okay with #3161 to 3.10.0 along with this?

@ann0see
Copy link
Member

ann0see commented Aug 30, 2023

Yes. I'll have a look at it.

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.

As it's equivalent to the previous behaviour approving.

@softins softins merged commit f0a4264 into jamulussoftware:main Aug 30, 2023
15 checks passed
@softins softins deleted the autobuild/overflow-fix-3161-alt branch August 30, 2023 13:31
@pljones pljones added this to the Release 3.10.0 milestone Aug 30, 2023
@ann0see ann0see added the needs documentation PRs requiring documentation changes or additions label Aug 30, 2023
@ann0see
Copy link
Member

ann0see commented Aug 30, 2023

Needs a changelog entry

@softins
Copy link
Member Author

softins commented Aug 30, 2023

Needs a changelog entry

I've lost track of what is now automated and what isn't. Does the "CHANGELOG" line in the above description make anything happen automatically?

@ann0see
Copy link
Member

ann0see commented Aug 30, 2023

There is a script which does the update but as the change log was manually edited (for merging multiple PRs) we need to add the last few entries manually.

@softins
Copy link
Member Author

softins commented Aug 30, 2023

we need to add the last few entries manually.

OK, do we know what else is missing, or does someone need to check? I'm happy to look, but won't have time till Friday.

@ann0see
Copy link
Member

ann0see commented Aug 30, 2023

That's fine. I believe only some Weblate PRs are missing

@pljones
Copy link
Collaborator

pljones commented Aug 31, 2023

@ann0see So can the "needs documentation" label be removed here?

@ann0see
Copy link
Member

ann0see commented Aug 31, 2023

It still needs a change log entry.

@pljones
Copy link
Collaborator

pljones commented Aug 31, 2023

That's part of the release process, not the documentation cycle. If something needs editing that we've already generated for the release, it should be documented under the release ticket, not here. Normally automatic Changelog generation would be used, it's only because we're in change freeze that something special is needed.

@ann0see
Copy link
Member

ann0see commented Aug 31, 2023

Ok. My error then.

@ann0see ann0see mentioned this pull request Aug 31, 2023
57 tasks
@ann0see ann0see removed the needs documentation PRs requiring documentation changes or additions label Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CodeQL is reporting "Security" issues in Jamulus source
3 participants