-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Conversation
FeralChild64
commented
Aug 13, 2023
•
edited
Loading
edited
- 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
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 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 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). |
There was a problem hiding this 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.
There was a problem hiding this 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.