Remove cpp-optparse from the rest of our project #4072

Sonicadvance1 · 2024-09-16T02:54:14Z

Based on #4070

First step: add an argument parser that handles /only/ what FEX needs for its tools.
Second step: Move everything over to that.
Third step: Delete cpp-optparse.

FEXLoader using the generated loader still.

Doesn't remove it from the full project since three tools still use it. FEXLoader argument loader is fairly special since we have a generator tied to it. This generator lets us have some niceties where everything ends up being a string literal without any sort of dynamic requirements. cpp-optparse has a systemic issue where it makes deep copies of /everything/ all the time. This means it has huge runtime costs and huge stack usage (4992 bytes). The new implementation uses 608 bytes of stack space (plus 640 if it actually parses a strenum). When profiling cpp-optparse with FEXLoader's argument it takes ~2,039,307 ns to parse the arguments! As a direct comparison this new implementation takes ~56,885 ns. Both sides not getting passed /any/ arguments, really shows how spicy this library is.

Fits our needs and /only/ our needs.

neobrain

Adding more comments here, most of which I think already apply to 4070.

neobrain · 2024-09-24T08:16:09Z

Source/Common/ArgumentLoader.cpp

+  }
+};
+
+bool NeedsArg(const OptionDetails* Details) {


Global functions used only locally should be static.

neobrain · 2024-09-24T08:17:19Z

Source/Common/ArgumentLoader.cpp

+  };
+
+  void ParseArgument(std::string_view Arg, std::string_view SecondArg, const OptionDetails* Details) {
+    std::invoke(Parser[FEXCore::ToUnderlying(Details->ParseType)], this, Arg, SecondArg, Details);


Why not just call the member pointer directly?

Suggested change

std::invoke(Parser[FEXCore::ToUnderlying(Details->ParseType)], this, Arg, SecondArg, Details);

(this->*Parser[FEXCore::ToUnderlying(Details->ParseType)])(Arg, SecondArg, Details);

neobrain · 2024-09-24T08:18:19Z

Source/Common/ArgumentLoader.cpp

+  _exit(1);
+}
+
+class ArgParser final {


This needs some documentation to explain why it exists and what's the difference from the confusingly similarly named ArgLoader.

neobrain · 2024-09-24T08:24:38Z

Source/Common/MicroArgParser.cpp

+}
+
+void MicroArgParser::PrintHelp(char** argv, std::span<ParseMember> ParseList) {
+  fextl::fmt::print("Usage: {} [options]\n", argv[0]);


This does not seem to handle line wrapping, e.g. FEXpidof now prints very long lines

-q Do not display matched PIDs to stdout. Simply exit with status of true or false if a PID was found -z Try to detect zombie processes - Usually zombie processes are skipped

instead of

-q Do not display matched PIDs to stdout. Simply exit with status of true or false if a PID was found -z Try to detect zombie processes - Usually zombie processes are skipped

(Also note the extra spaces on the left. Not sure where those are coming from. Both versions seem overly padding-happy, not sure if there's some sort of standard that says there should be at least N characters of whitespace here. Conversely, FEXRootFSFetcher --help doesn't vertically align the option descriptions for the lengthy --force-ui=[default,tty,zenity], though that seems to be the only setting with that issue.)

neobrain · 2024-09-24T08:28:39Z

Source/Tools/FEXGetConfig/Main.cpp

+    {FEX::MicroArgParser::ParseMember::Type::Bool, {}, "--current-rootfs", "Print the directory that contains the FEX rootfs. Mounted in the case of squashfs", "0"},
+    {FEX::MicroArgParser::ParseMember::Type::Bool, {}, "--tso-emulation-info", "Print how FEX is emulating the x86-TSO memory model.", "0"},
+    // Defaults
+    {FEX::MicroArgParser::ParseMember::Type::Bool, "-v", "--version", "show program's verison number and exit", "0"},


verison is spelt wrong here and in a couple of other places.

neobrain · 2024-09-24T08:30:06Z

Source/Tools/pidof/pidof.cpp

+    {FEX::MicroArgParser::ParseMember::Type::StringArray, "-o", {}, "Ignore processes with matched pids", {}},
+
+    // Defaults
+    {FEX::MicroArgParser::ParseMember::Type::Bool, "-v", "--version", "show program's verison number and exit", "0"},


show should be spelt Show for consistency with other descriptions.

neobrain · 2024-09-24T08:39:34Z

Source/Common/MicroArgParser.cpp

+    }
+  }
+
+  ExitWithError(fextl::fmt::format("'{}' Not in list of choices '{}'\n", Arg, Member->Choices));


Suggested change

ExitWithError(fextl::fmt::format("'{}' Not in list of choices '{}'\n", Arg, Member->Choices));

ExitWithError(fextl::fmt::format("Invalid argument '{}'. Valid choices are '{}'\n", Arg, Member->Choices));

or better yet:

Suggested change

ExitWithError(fextl::fmt::format("'{}' Not in list of choices '{}'\n", Arg, Member->Choices));

ExitWithError(fextl::fmt::format("Invalid argument '{}' for --TODO. Valid choices are '{}'\n", Arg, Member->Choices));

neobrain · 2024-09-24T08:40:51Z

Source/Common/MicroArgParser.cpp

+  ExitWithError(fextl::fmt::format("'{}' Not in list of choices '{}'\n", Arg, Member->Choices));
+}
+
+void MicroArgParser::Parse(int argc, char** argv) {


Invoking multiple short options at once seems broken now:

./Bin/FEXpidof -sq Error: Unknown argument: -sq

neobrain · 2024-09-24T08:42:56Z

Source/Common/MicroArgParser.h

+    }
+
+  private:
+    fextl::vector<std::string_view> _Value;


Identifiers starting with an underscore look out of place here.

Also, this can be an std::optional<vector<string_view>> instead to avoid having two variables to manage the same state.

neobrain · 2024-09-24T08:51:05Z

Source/Tools/pidof/pidof.cpp

+    {FEX::MicroArgParser::ParseMember::Type::StringArray, "-o", {}, "Ignore processes with matched pids", {}},
+
+    // Defaults
+    {FEX::MicroArgParser::ParseMember::Type::Bool, "-v", "--version", "show program's verison number and exit", "0"},


The apostrophe s isn't needed here (and actually sounds slightly wrong to me due to lack of preceding article):
program's version -> program version

neobrain · 2024-09-30T18:07:05Z

Source/Common/ArgumentLoader.cpp

+    if (Arg == "--") {
+      // Special case break. Remaining arguments get passed to guest.
+      ++ArgParsed;
+      break;
+    }


For tools other than FEXLoader, are we checking and failing if "--" is used by accident (e.g. by slipping in a space)?

Sonicadvance1 force-pushed the remove_cpp_optparse_therest branch 4 times, most recently from 6d8a867 to 488a472 Compare September 16, 2024 04:49

Sonicadvance1 added 8 commits September 18, 2024 10:53

unittests: Be more strict with -- separator

b6c9a5b

Common: Add small argument parser

d1ed153

Fits our needs and /only/ our needs.

FEXGetConfig: Move over to MicroArgParser

104ab4a

FEXRootFSFetcher: Move over to MicroArgParser

748fdbf

FEXServer: Move over to MicroArgParser

ab561d3

FEXpidof: Move over to MicroArgParser

91c1b80

Common: Remove cpp-optparse from the project

3f36125

Sonicadvance1 force-pushed the remove_cpp_optparse_therest branch from 488a472 to 3f36125 Compare September 18, 2024 17:55

neobrain reviewed Sep 24, 2024

View reviewed changes

neobrain reviewed Sep 30, 2024

View reviewed changes

Sonicadvance1 closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cpp-optparse from the rest of our project #4072

Remove cpp-optparse from the rest of our project #4072

Sonicadvance1 commented Sep 16, 2024

neobrain left a comment

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 24, 2024

neobrain Sep 30, 2024

	std::invoke(Parser[FEXCore::ToUnderlying(Details->ParseType)], this, Arg, SecondArg, Details);
	(this->*Parser[FEXCore::ToUnderlying(Details->ParseType)])(Arg, SecondArg, Details);

	ExitWithError(fextl::fmt::format("'{}' Not in list of choices '{}'\n", Arg, Member->Choices));
	ExitWithError(fextl::fmt::format("Invalid argument '{}'. Valid choices are '{}'\n", Arg, Member->Choices));

Remove cpp-optparse from the rest of our project #4072

Remove cpp-optparse from the rest of our project #4072

Conversation

Sonicadvance1 commented Sep 16, 2024

neobrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment