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

[[unlikely]] ASSERT and minor ASSERT usage changes #10908

Merged
merged 1 commit into from Mar 3, 2023

Conversation

Minty-Meeo
Copy link
Member

This PR doubles as a way to ask what are Dolphin Emulator's plans for moving to C++20? Is the issue that GCC still defaults to C++17 in version 12? Other compiler support reasons?

And just how far should [[likely]] and [[unlikely]] be taken? Nullptr checks seem like obvious candidates. Any path that leads to a PanicAlert as well. There are a lot of switch case defaults which only exist to catch stuff which should never happen.

@lioncash
Copy link
Member

what are Dolphin Emulator's plans for moving to C++20?

Honestly, I've wanted to move to it for ages now

And just how far should [[likely]] and [[unlikely]] be taken?

Using [[unlikely]] inside of asserts is fine, but for other cases, it's best to have profiling info to back other applications of it (especially for [[likely]], otherwise it makes existing code more noisy (when applied liberally) for (in most cases) negligible benefit

@Minty-Meeo
Copy link
Member Author

Nice, we are finally on C++20!

@shuffle2
Copy link
Contributor

is this mergeable now?

Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this had actually been merged a while ago. Looks good to me (though it should maybe be rebased to make sure everything still builds properly, and it looks like there's a cassert include in CheatSearchWidget.cpp that's unused (not sure if that already existed).

and other ASSERT usage changes
@Pokechu22 Pokechu22 merged commit a93b5a4 into dolphin-emu:master Mar 3, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants