-
Notifications
You must be signed in to change notification settings - Fork 233
Add Regex Engine to FEX for config option loading #5120
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm unsure on a full regex engine, I think a very naive glob impl might be enough. albeit this is very neat |
Sonicadvance1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the concern about the licensing of the regex engine used, I'm fine with this type of change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure on a full regex engine, I think a very naive glob impl might be enough. albeit this is very neat
I'd like to echo this - adding a full-blown regex engine without documentation and bare-bones unit tests is overkill for the concrete feature this PR intends to add. (Though I do appreciate that there are tests :) )
Could you focus the implementation on pure wildcards (* instead of .*) instead, without unions and the other features that we don't need for config matching? That should allow for throwing away most of the complexity here, notably the use of NFAs.
This doesn't necessarily imply a rewrite (up to you). Deleting the relevant code and taking a quick pass over what's left would yield an equally simpler result.
|
I've gone ahead and removed some features like unions, escapables, question, plus. Please let me know if this is sufficient for the PR |
neobrain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much easier to read :)
Adding a few more comments, no big changes though!
| @@ -0,0 +1,33 @@ | |||
| #include "FEXCore/fextl/string.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add these?
- Empty wildcard at the beginning/end:
CHECK(Regex::matches("a*", "a"));
CHECK(Regex::matches("*a", "a"));
CHECK(Regex::matches("*a*", "a"));- wildcard in the middle:
CHECK(Regex::matches("test*pattern", "test__pattern"));- multiple wildcards:
CHECK(Regex::matches("test*pattern*more", "test__pattern__more"));
CHECK(Regex::matches("test**pattern", "test_pattern"));- Also might be worth adding a test for something like
Regex::matches("a*", "a*"). Not exactly relevant to the use case, but should be covered by the helper implementation.
| InterruptableConditionVariable | ||
| StringUtils) | ||
| StringUtils | ||
| Regex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no longer a Regex engine, so rename to something like WildcardMatch(er)?
| // Matches the first and then get out | ||
| // Needs PR review on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you need feedback on?
|
|
||
| ListApplier(ConfigList); | ||
|
|
||
| const json_t* RegexList = json_getProperty(json, "RegexConfig"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this property to AppOverrides? It can be used without wildcards after all.
| auto ListApplier = [&Config, &Func](const json_t* jsonList) { | ||
| for (const json_t* ConfigItem = json_getChild(jsonList); ConfigItem != nullptr; ConfigItem = json_getSibling(ConfigItem)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline lambdas are hard to make sense of as a reader since they require a lot of jumping around to get a basic grasp of what a function does.
In this case, one way to improve the structure is to first check the RegexList and collect all matching json_ts into an fextl::vector. Then, we can turn the existing loop into something like this:
| auto ListApplier = [&Config, &Func](const json_t* jsonList) { | |
| for (const json_t* ConfigItem = json_getChild(jsonList); ConfigItem != nullptr; ConfigItem = json_getSibling(ConfigItem)) { | |
| for (auto ConfigBlock : ConfigBlocks) { | |
| for (const json_t* ConfigItem = json_getChild(ConfigBlock); ConfigItem != nullptr; ConfigItem = json_getSibling(ConfigItem)) { |
|
Oh, you'll also want to run clang-format-19 on any newly added code. (I think most of us have our IDEs set up to auto-format on save.) |
gotacha i'll install it |
Hi there, i'm opening the PR to add a small regex engine to the Fex codebase. The motivation is to help dynamic modify some config option based on the name of the config file. Please see the following screenshot for more context.
.
The regex engine is an NFA based one, implemented from the dragon book 2nd ed.
Implementation is in
FEXCore/Source/Utils/Regex.cpp.Test case is
unittests/APITests/Regex.cpp.I took the liberty to refactor and reuse a bit of the code in parsing the json. The traversing of a json list of pair of strings is now a helper function called ListApplier that accepts a
json_t.The default behavior is that the config loader goes through each regex and see if it matches the config name, if it does, it applies the config option specified and then stops checking anymore regex (this can change based on the need and reviews of the PR).
I'm not sure where should i add the test case for the new config loader. I tested locally via
/home/ubuntu/.fex-emu/Config.jsonand it works, would love some suggestions