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

CMakeLists: Add Clang's Specific WShadow Diagnostics #12625

Merged

Conversation

mitaclaw
Copy link
Contributor

@mitaclaw mitaclaw commented Mar 8, 2024

For Clang parity with GCC. GCC is justified in detecting this, as it is technically correct when using decltype, but I believe it would be better to disable the warning if it were an option, hence my comment.

@mitaclaw
Copy link
Contributor Author

mitaclaw commented Mar 8, 2024

This change is motivated by #12610 being necessary after Clang failed to notify me of these GCC warnings.

@mitaclaw
Copy link
Contributor Author

mitaclaw commented Mar 8, 2024

Clang also has -Wshadow-field-in-constructor, another kind of shadowing Clang does not detect by default which GCC detects as part of its -Wshadow option. Would it be worth enabling that as well for parity, seeing as we already avoid it in many places?

@AdmiralCurtiss
Copy link
Contributor

Yes, please add -Wshadow-field-in-constructor. I'd also remove the comment, though I get where you're coming from.

@mitaclaw mitaclaw changed the title CMakeLists: Add -WShadow-uncaptured-local CMakeLists: Add Clang's Specific WShadow Diagnostics Mar 9, 2024
@mitaclaw
Copy link
Contributor Author

mitaclaw commented Mar 9, 2024

Looking again at Clang's documentation, there is also -WShadow-field to catch members shadowed during inheritance.
https://releases.llvm.org/5.0.0/tools/clang/docs/DiagnosticsReference.html#wshadow-field
Enabling this, it doesn't seem to happen anywhere in Dolphin's codebase. Is it worth adding as well?

@AdmiralCurtiss AdmiralCurtiss merged commit adac827 into dolphin-emu:master Mar 9, 2024
11 checks passed
@AdmiralCurtiss
Copy link
Contributor

Oh, sorry didn't see that comment. If you want, but it seems pretty unlikely to be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants