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

static const std::regex #11902

Merged
merged 1 commit into from Jun 8, 2023

Conversation

Minty-Meeo
Copy link
Contributor

@Minty-Meeo Minty-Meeo commented Jun 8, 2023

I noticed that a few usages of std::regex in the codebase were suboptimal. I would prefer to make these static const globals in their respective compilation units, but that is sometimes hard to sell despite the performance savings.
Also, a lot of std::regex usage comes from baffling design decisions in the debugger parts of Dolphin. Hopefully in the future I or someone else can rework the debugger to use raw UGeckoInstructions rather than disassembled instructions, but it seems like a huge task.

@Minty-Meeo
Copy link
Contributor Author

Weird MSVC bug.

@BhaaLseN
Copy link
Member

BhaaLseN commented Jun 8, 2023

Are any of those supposed to be anchored to start of line (^) and/or end of line ($)? Thats usually a thing I look out for at work, since many regex do match like that, but don't do what they're supposed to do (which is match if and only if the exact format matches)

@AdmiralCurtiss
Copy link
Contributor

You do realize that a static in-function variable is still a global, right? Though in this case I don't think it's a problem because it's not a mutable global. I'm also kinda surprised these can't be constexpr?

@lioncash
Copy link
Member

lioncash commented Jun 8, 2023

Yeah, making these static is fine imo, since they're non-mutable. The standard regex interface is quite lacking in various ways and is known to be a sore-spot (and I don't think it's had any substantial changes since its introduction).

Commit could probably include the rationale as well in its description (no need to reconstruct the regex over and over again)

@Minty-Meeo
Copy link
Contributor Author

@AdmiralCurtiss In-function static variables are just-in-time constructed on first usage. This is kept thread safe with a lock guard, which is costly for an unchanging static const variable. Thus, to fully optimize std::regex, it should constructed at the global scope (constructed before main runs), but statically to keep the name from leaking out of your compilation unit during link-time. std::regex is not constexpr yet primarily because it deals with std::locale stuff, though there is a proposal that attempts to work around this.
@BhaaLseN start of line / end of line anchors are only enabled in ECMAScript when a std::regex is constructed with the std::regex_constants::multiline flag. Every usage of std::regex in the codebase appears to works on pre-isolated lines, so multiline mode seems irrelevant here. Matching the entire input is what std::regex_match does in this case, as opposed to std::regex_search which can find partial matches.
@lioncash having recently finished a personal project which used a ton of std::regex, I couldn't agree more about it being neglected. It baffles me that a proposal simply adding std::string_view support to std::regex has been ignored for over four years. It's a shame.

So, no strong desire from the reviewers for static const global variables?

@lioncash
Copy link
Member

lioncash commented Jun 8, 2023

Personally I would just keep them as static const locals. Not really a fan of making them global/namespace-scope. I think them being kept local is more adequate, since these are used with mostly non-essential/core features for the regular user (like debugging and whatnot. UI-side being very negligible, considering all the other stuff going on around them), so instantiating them only when necessary seems like a decent tradeoff.

std::regex has a relatively expensive constructor, and these are unchanging regexes.
@AdmiralCurtiss AdmiralCurtiss merged commit e8c87dc into dolphin-emu:master Jun 8, 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