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

Another batch of PVS Studio fixes #2756

Merged
merged 15 commits into from
Aug 14, 2023
Merged

Another batch of PVS Studio fixes #2756

merged 15 commits into from
Aug 14, 2023

Conversation

FeralChild64
Copy link
Collaborator

@FeralChild64 FeralChild64 commented Aug 13, 2023

  • fixed 30 more warnings
  • regarding loguru changes - it seems the project is still being developed (like few commits per year), so once this gets merged I intend to create a GitHub Issue in the loguru project

@FeralChild64 FeralChild64 added the cleanup Non-functional changes that simplify, improve maintainability, or squash warnings label Aug 13, 2023
@FeralChild64 FeralChild64 self-assigned this Aug 13, 2023
@FeralChild64 FeralChild64 marked this pull request as ready for review August 13, 2023 16:37
@FeralChild64 FeralChild64 changed the title [Work-in-progress] Another batch of PVS Studio fixes Another batch of PVS Studio fixes Aug 13, 2023
@kcgen
Copy link
Member

kcgen commented Aug 13, 2023

The loguru author hasn't given enough people rights to keep the project actively maintained, so it's gotten in a bad state with lots of forks.

Their own master branch fails in their own CI, and they haven't even accepted PRs to get it passing (the only green PRs in their queue include the fix to get their CI working again).

I've got queued PRs that fix real issues, like:

And sanitizer cleanups:

Back when we adopted it, I forked the project here: https://github.com/dosbox-staging/loguru, and then applied the above fixes to both our fork and the upstream (via PRs, which are stills stagnant).

Currently our fork is still ahead by ~35 commits (and you can see 245 people have launched new forks from ours!):

emilk/loguru@master...dosbox-staging:loguru:master

There are 25 other commits we haven't pulled in versus upstream because they add more warnings or static analysis problems.


So I would suggest PRing your good Loguru fixes against both Staging loguru and upstream's Loguru. I've added you as admin to the Loguru project here, so you can simply clone, commit, and push.

Once Staging org's Loguru has your fixed commits, then simply copy that new source .cpp/.hpp file into Staging's repo as a single commit "Sync loguru with upstream".

If the upstream loguru does accept your PR at some point, then we just have to rebase against upstream here, and we can continue to keep our loguru project in-sync with little effort.


The seems convoluted (and it /is/ a bit more work), but the benefit is that we can easily keep our fork of rebased against upstream, if/when it gets updated.

If the loguru commit goes directly into Staging here, then we have divergent code trees versus the upstream projects (both the Staging-loguru repo and it's parent loguru repo).

Copy link
Collaborator

@weirddan455 weirddan455 left a comment

Choose a reason for hiding this comment

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

These all look like good low risk changes. I like seeing those warnings continue to get lower. Nice job.

src/dos/program_autotype.cpp Show resolved Hide resolved
Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Very nice cleanup, @FeralChild64.
Was able to review all the commits; looks good to me.

@FeralChild64 FeralChild64 merged commit e10282e into main Aug 14, 2023
50 checks passed
@FeralChild64 FeralChild64 deleted the fc/static-analysis-2 branch August 20, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non-functional changes that simplify, improve maintainability, or squash warnings
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants