dxvk icon indicating copy to clipboard operation
dxvk copied to clipboard

[dxvk/d3d11] Static analysis corrections and nits

Open WinterSnowfall opened this issue 10 months ago • 6 comments

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.

WinterSnowfall avatar Mar 08 '25 21:03 WinterSnowfall

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.

pchome avatar Mar 08 '25 23:03 pchome

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.

WinterSnowfall avatar Mar 09 '25 00:03 WinterSnowfall

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.

doitsujin avatar Mar 09 '25 00:03 doitsujin

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

pchome avatar Mar 09 '25 00:03 pchome

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 :sweat_smile:. 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.

WinterSnowfall avatar Mar 09 '25 00:03 WinterSnowfall

@WinterSnowfall you can introduce them one-by-one ;) https://github.com/doitsujin/dxvk/pulls?q=is%3Apr+tidy+is%3Aclosed

pchome avatar Mar 09 '25 00:03 pchome

Kind of obsolete at this point with all the refactoring going on. I can always run static analysis again on master if deemed useful, but most of the remaining stuff here can be used as hints/reference anyway.

WinterSnowfall avatar Jun 27 '25 19:06 WinterSnowfall