dolphin
dolphin copied to clipboard
dolphin-emu
Coding Style Conformance
Crazy idea I had for a pull request. Upon closer inspection of the Coding Style Guidelines, I may have overdone it (it only is intended to be followed within for loops). Willing to scale back as needed.
Why??????
Quickly skimming, most (if not all) of these are just POD types, and there is no performance difference between postfix/prefix operators.
Even if our coding guidelines do say prefix is "preferred", You are just polluting git history.
Polluting git history is a bit harsh, its a single commit and not that many bytes. We have coding style guidelines for a reason, and we've done similar changes in the past.
Previous mass changes like this were typically done so we could get the advantage of running automated tooling, like clang format on every commit. Which was well-worth it.
Nothing like that is happening here. There is no practical upside, it doesn't allow us to run any tooling. It doesn't make the code easier to read. It doesn't improve performance or reliability. All it really does is make blame harder.
If it bothers you so much that the existing code doesn't match the style guidelines, a PR changing the style guidelines would be better.
If it makes blame harder, then we can add it to the .git-blame-ignore-revs file.
Yes, that helps mitigate the issues. But my question is still "Why??????"
And in my personal opinion, "to better conform to style guidelines" is not a good enough answer.
Got this commit down from 2,326 lines changed to 1,650 lines changed by narrowing it to just for loops, plus I added the commit's hash to the .git-blame-ignore-revs file like @Zopolis4 suggested.
I made this pull request because I believed this menial task had only not been done because it was uninteresting. Would Dolphin Emulator become worse if these changes are accepted? I think being true to style guidelines makes a good example to follow.
There is a grand total of two 'for (;;)' loops remaining in all of Dolphin's codebase. Might be worth wrapping it into this PR.
I wish I understood what is causing FifoCl to have extremely faint differences. My only guess is I also changed the generated shaders to use pre-increment in their for loops as well.
It's doing that to other PRs too. Note how only one builder is affected - there's something up with that particular builder.
There seems to be some nuance to the continued usage of the NULL macro in some places. Is it still better to switch them all to nullptr?
My only guess is I also changed the generated shaders to use pre-increment in their for loops as well.
Which is part of my point. Now you are paranoid that this auto-generated style-only change had a behaviour difference. It's hard to review because it's so mind-numbing boring to even look at the diff logs. (But in this case it's a driver bug with the fifo log runner that's been causing random issues recently)
Commit's aren't free. They are really, really cheap, but they aren't free.
When this gets merged, there will be open PRs, WIP branches or out-of-tree patchsets that will now have to put in a tiny bit more effort rebase or merge, or cherry-pick over it. If someone's blame tooling doesn't know about .git-blame-ignore-revs, or it bugs out; It will add a tiny bit more time to their coding.
It's it worth complaining about, probably not. I might have been in a bit of a bad mood when the PR popped up. But I just want people to be a little bit more considerate before making large meaningless mass style changes like this. One probably isn't too bad, but too many can have a negative effect on other people working in a project.
BTW, my view here is limited to just the prefix loop iterators. Mostly because it's a massive change that effects many lines of code; and in my opinion has absolutely zero benefits (and nobody has even tried to convince me other wise).
I'm fine with the other stuff you have since tacked onto this PR. Empty loop bodies, infinite loops, nullptr instead of NULL. These all have meaningful readability improvements. Though they are now hidden inside a PR with meaningless noise.
Paranoid is again, a bit harsh. It is reasonable for one not familiar with fifoCI's bountiful false diffs for one to assume they may have been at fault.
Attacking .git-blame-ignore-revs fails to realise that if someone is not using it then they already have impossible diffs, this won't change anything.
Most PR's that need a rebase already need one. Dolphin PR's building up and growing stale is an already present issue that is not due to style changes. (i.e. there were 200 pr's outstanding in around September 2021, and now there are 300).
If you don't want people to make style changes, change the style. Minty won't be the first to reasonably think that it is good for Dolphin's code to match its style.
Attacking
.git-blame-ignore-revsfails to realise that if someone is not using it then they already have impossible diffs, this won't change anything.
The situation without .git-blame-ignore-revs isn't impossible at all, it's just a bit annoying. What this PR does it make it a bit more annoying. Yes, the negative effect of this isn't all that large, but this PR doesn't have a large enough positive effect to outweigh it in my opinion. Having i++ in a bunch of places really isn't much of a problem, even if it's not the preferred style.
If you don't want people to make style changes, change the style. Minty won't be the first to reasonably think that it is good for Dolphin's code to match its style.
Nowhere in the coding guidelines says 'never user postincrement' or 'always use preincrement' or anything definitive like that. In fact, the specific line we're talking about is:
- The preferred form of the increment and decrement operator in for-loops is prefix-form (e.g.
++var).
'preferred form' does not read to me as 'always only use this form in all code including code that is already in the codebase'.
"Preferred" aka every pull request that misuses post-increment is told to change it during code review, and changes which are adjacent to post-increment usage should "fix it while you're there."
"Preferred" aka every pull request that misuses post-increment is told to change it during code review, and changes which are adjacent to post-increment usage should "fix it while you're there."
I think working incrementally like this is less brutal and less likely to have side-effects. Touching that much code might be armful if we can't bisect where a regression (from one of the many places this PR changes) come from. That's even more dangerous that the contributors involved in the code (that was changed by this PR) might not be as active as they were before.
Alright I don't have time right now to try this until tonight, but here is my idea: Assuming a compiler is 100% deterministic, and assuming every one of the postfix to prefix changes should have ZERO EFFECT on the compiled result (except for shadergen strings and one or two loops I think actually used iterators), a build of dolphin-emu before and after the changes to for loops should be byte-for-byte identical. If I cannot prove this, I will delete this PR on the spot, because I am sick of arguing why this is should happen.
I took the liberty of doing this, and there is a single byte change between f07e8fed1c3caa78206f656292c270c74de984b8 and de5499f3bde813a2749204c8dfa14d7d49a5c073, which I am pretty sure is caused by revision change. At 0x00000028, the byte changes from 0x18 to 0x20. This is using g++ (Debian 11.3.0-5) 11.3.0 and gcc (GCC) 12.0.1 20220118 (experimental).
If you don't want people to make style changes, change the style.
I don't really want to argue against this point in the style guide.
Perhaps what we should probably do is extend the style guide to add guidelines for commits, commit message and pull request structure. We have preferences around avoiding fix-up commits (rebasing away) that should be written down somewhere, and I do wish people would make more effort to keep their commit subject lines below the 72 char threshold where GitHub truncates it.
With that, we could consider a guideline along the lines of:
Think carefully before creating large large automated style/formatting changes that touch dozens of files; Make sure there is a decent justification for the changes.
Though that might be going too far. I don't want to discourage style commits.
I do like the idea of adding guidelines for git related things, I think in regards to the mass style changes there arent that many left, and I think handling them on a case by case basis is better.
Apologies for missing my own deadline.
Using GCC 12.1.0 on Linux for the Release build variant, I was able to prove deterministic compilation with a control test (yay). Compiling this PR's branch as-is produced several bytes of changed instructions when compared to the build of the base commit, not just changes to strings. This could be from anything, so I had to try something a bit more sophisticated in a testing branch to see what was up. After compiling this test branch, there were still byte differences. However, upon closer inspection, every single one resides in the symbol table! So, at least for code relevant to Dolphin Emulator on Linux compiled with GCC, post-increment to pre-increment changes outside of shadergen strings is 1:1 matching. I know this because the first commit of two in the testing branch is identical to the first commit of this PR. Next, I need to check Windows builds. I will be unable to confirm any ARM-exclusive code changes, so that will still need manual review.
For MSVC, the same test branch produces countless byte differences. From what I can tell, it is all slightly different jump instructions (as if everything was shifted 0x30 bytes backward in memory) and maybe slight stack allocation differences. So much for using determinism for the Windows-specific burden of proof.
If you are willing to trust my judgement, I scanned every single file changed by the increment/decrement commit for preprocessor blocks and/or obvious platform exclusivity. These files should be the only ones not proven via the GCC on Linux experiment:
- Source/Android/jni/Cheats/ARCheat.cpp * Android exclusive file
- Source/Android/jni/Cheats/GeckoCheat.cpp * Android exclusive file
- Source/Android/jni/Cheats/PatchCheat.cpp * Android exclusive file
- Source/Core/AudioCommon/WASAPIStream.cpp * Windows exclusive file
- Source/Core/Common/Arm64Emitter.cpp * ARM exclusive file
- Source/Core/Common/CompatPatches.cpp * Windows exclusive file
- Source/Core/Common/Semaphore.h * __APPLE__ preprocessor section
- Source/Core/Common/TraversalServer.cpp * DEBUG preprocessor section
- Source/Core/Core/DSP/Interpreter/DSPInterpreter.cpp * PRECISE_BACKLOG preprocessor section
- Source/Core/Core/HW/EXI/BBA/TAP_Win32.cpp * Windows exclusive file
- Source/Core/Core/HW/WiimoteEmu/Speaker.cpp * WIIMOTE_SPEAKER_DUMP preprocessor section
- Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp * SHOW_HISTORY preprocessor section
- Source/Core/Core/PowerPC/Jit64/Jit.cpp * JIT_LOG_GPR and JIT_LOG_FPR preprocessor sections
- Source/Core/Core/PowerPC/JitArm64/Jit.cpp * ARM exclusive file
- Source/Core/Core/PowerPC/JitArm64/JitArm64_LoadStore.cpp * ARM exclusive file
- Source/Core/Core/PowerPC/JitArm64/JitArm64_SystemRegisters.cpp * ARM exclusive file
- Source/Core/Core/PowerPC/JitArm64/JitArm64_Tables.cpp * ARM exclusive file
- Source/Core/DolphinQt/FIFO/FIFOAnalyzer.cpp * INCLUDE_HEX_IN_PRIMITIVES preprocessor section
- Source/Core/UICommon/X11Utils.cpp * HAVE_XRANDR preprocessor section
- Source/Core/VideoBackends/D3D/D3DNativeVertexFormat.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D/D3DState.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D/D3DVertexManager.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/D3D12PerfQuery.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/D3D12Renderer.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/D3D12SwapChain.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/DX12Context.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/DX12Texture.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/DX12VertexFormat.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/DescriptorAllocator.cpp * Windows exclusive file
- Source/Core/VideoBackends/D3D12/DescriptorHeapManager.cpp * Windows exclusive file
- Source/Core/VideoCommon/TextureDecoder_Generic.cpp * 0 preprocessor section
- Source/Core/VideoCommon/TextureDecoder_x64.cpp * CHECK preprocessor section
- Source/Core/VideoCommon/VertexLoaderARM64.cpp * ARM exclusive file
- Source/DSPSpy/main_spy.cpp * Not compiled normally
- Source/DSPTool/DSPTool.cpp * Not compiled normally
- Source/UnitTests/Core/PowerPC/JitArm64/MovI2R.cpp * ARM exclusive file
And I have already checked these all myself, but do look for yourself.
Had to rebase due to 7d2d5d914bf3cacbecbb0bf8a642b616744aad54 slightly changing /Source/Core/Common/Hash.cpp. Otherwise, everything is exactly as it was, including the path to reviewing this PR.