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

[dxvk/d3d11] Static analysis corrections and nits #4748

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WinterSnowfall
Copy link
Contributor

So, uhm, I had some time to kill today and threw Cppcheck at the rest of the project basically. Feel free to ignore this PR until you're feeling like having a look. There's the usual mix of uninitialized variables, initializer list moves and a few derps static analysis helped uncover.

I'll be running a build of master + #4743 + this PR from now on to test, but I've already thrown this at a few d3d11 games and it looks good, both functionally and from a performance standpoint.

@pchome
Copy link
Contributor

pchome commented Mar 8, 2025

FYI: Some time ago I played with clang-tidy, it was possible with winelib build and native clang on linux.
But now with e.g. native build or llvm-mingw it should be as easy as running clang-tidy --checks="-*,bugprone*" on build root with compile_commands.json generated.

afaik cppcheck support clang-tidy, also meson support clang-tidy integration as separate target ... if you want to go deeper.

@WinterSnowfall
Copy link
Contributor Author

if you want to go deeper.

Yeah, no :P. But thanks for the insight, I might have a use for that on other stuff I'm working on.

@doitsujin
Copy link
Owner

doitsujin commented Mar 9, 2025

afaik cppcheck support clang-tidy, also meson support clang-tidy integration as separate target ... if you want to go deeper.

I really don't want to review some tool making random changes to ~150k lines of code. This project is 7 years old, and yes the code quality is arguably bad in many places, but you're also not going to just magically fix that in a day.

Honestly, unless this turns up something actually interesting like that one dxso binding derp, I'd prefer if we could just improve things like variable/member initialization incrementally when related code is being touched anyway, which is something I've started doing on the backend side of things as well in the past months. Otherwise this is kinda starting to feel like changing stuff for the sake of it and distracts from actual problems.

@pchome
Copy link
Contributor

pchome commented Mar 9, 2025

I specifically wrote it as --checks="-*,bugprone*".
Mean -* disable all, bugprone* enable all starting with bugprone. You'll get few of them maybe, mostly false positive.

idk, want init? set *init*. https://clang.llvm.org/extra/clang-tidy/checks/list.html

@WinterSnowfall
Copy link
Contributor Author

Honestly, unless this turns up something actually interesting like that one dxso binding derp,

I think the stuff in dxvk_memory.cpp and dxvk_context.h is worth it at least.

I'd prefer if we could just improve things like variable/member initialization incrementally when related code is being touched anyway

Fair, those were mostly a collateral from fixing uninitialized variable warnings which is really the entire intended point of this exercise. But you're right in the sense that if any of this would have been truly problematic, it would have exploded by now.

Otherwise this is kinda starting to feel like changing stuff for the sake of it and distracts from actual problems.

Definitely meant it to be low priority, sorry if it was too much of a distraction 😅. You can cherry pick whatever you feel is worth addressing at any time - I've pretty much covered all the static analysis errors and warnings in the two PRs. Everything else (style, portability etc.) was way way more pedantic and didn't even bother with it.

@pchome
Copy link
Contributor

pchome commented Mar 9, 2025

@WinterSnowfall WinterSnowfall force-pushed the staticanalysis2 branch 2 times, most recently from c46b2e9 to 6ce3318 Compare March 9, 2025 11:31
@WinterSnowfall WinterSnowfall marked this pull request as ready for review March 16, 2025 15:24
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.

3 participants