OpenXRay
command line options, started transition (#570)
I wish to discuss these changes. New system for handling command line options.
While transitioning to the news system, I noticed that this approach is very inconvenient when adding command line options that are used across different subsystems. One has to define a public variable and declare it external in other places where it's used, keeping in mind that the unit where external declaration is used must be compiled after the unit where the variable is defined. This is not very readable and extendable, although not many options need to be used across different modules.
Another option is to have a public variable containing a map of command line options and access it like so:
command_line_options_map.getvalue("-myoption").
This could lead to some overhead.
This PR is just for discussion.
The key files to look at:
src/xrCore/CLOptions.h
src/xrCore/CLOptions.cpp
src/xrCore/xrCore.cpp
src/xrCore/LocatorAPI.cpp
src/xrEngine/main.cpp
src/xr_3da/entry_point.cpp
What you've done is just re-invention of cxxopts
The initial code has an issue (e.g. MODEL::build()) where an option gets checked every function call. In this case, we'd better cache its value in the class constructor/init function. With this, I don't care about any overhead added by shared map/string/whatever since all cmd line args will be parsed at the early module initialization only.
What you've done is just re-invention of cxxopts
This isn't my design, but I haven't heard of cxxopts
The initial code has an issue (e.g.
MODEL::build()) where an option gets checked every function call. In this case, we'd better cache its value in the class constructor/init function. With this, I don't care about any overhead added by shared map/string/whatever since all cmd line args will be parsed at the early module initialization only.
Hm, I wouldn't like to go through all classes and cache the relevant options as this would require a lot of refactoring. Although this could be a little neater solution.
Edit: I've looked at cxxopts and their designd seems to be quite different from the design in this patch.
I think declaring a command line option as global variable in a header is dangerous, since a header may be included in several different compilation units and there will be multiple copies of that global variable.
I think this PR is almost ready for the second review. There are two things that I'm not sure of:
- I don't think it's not sufficient to just add command_line_key.h into xrCore.h (see above comment)
- I'm thinking about getting rid of dynamic memory allocation. I think a static variable of 20 bytes is enough to store the name of the key and the oxr string2048 or string4096 to store the provided int/string argument. This would waste a few Kb of memory. If this is ok then I'll just use static variable. command line keys like
-startisn't an issue since this is a separate case and needs another approach like passing a string with new keys to the command_line_key parser once again - we should discuss this case more.
2\. It is possible to remove memory allocations and use `pcstr`. Please, use `pcstr` without memory allocations.
Wait, I don't get it. I thought I'm supposed to use a string stored in the command_line_key class like string2048?
If it's just a pointer, then I will have to to allocate memory for that string.
Keep in mind that I'll need to modify the provided string (option name) to prepend a -.
As for the key value (if it requires a string value) I can just copy the pointer to the string from char **argv. I'm not sure
if windows provides null-terminated strings so that could be a problem.
2\. It is possible to remove memory allocations and use `pcstr`. Please, use `pcstr` without memory allocations.Wait, I don't get it. I thought I'm supposed to use a string stored in the command_line_key class like string2048? If it's just a pointer, then I will have to to allocate memory for that string. Keep in mind that I'll need to modify the provided string (option name) to prepend a
-.As for the key value (if it requires a string value) I can just copy the pointer to the string from
char **argv. I'm not sure if windows provides null-terminated strings so that could be a problem.
Ah, yea. You need to store the value... For now, let's try to use string1024.
Keep in mind that I'll need to modify the provided string (option name) to prepend a -.
To avoid modification of the original string, you can use temporary string on the stack in the initialization function.
To avoid modification of the original string, you can use temporary string on the stack in the initialization function.
Hm, I could just store the key name without the - and imply that it's there when parsing the command line, but it's still
simpler and more readable, explicit for the programmers to store the key name as a string32 in the command_line_key class.
It's not that much resources, although could be redundant. Either way is possible.
I also did not want to assume that arguments to the command_line_key constructor have static lifetime.
This could be useful if you're adding a key and then reparsing a new command line again. I think there are cases when
they dynamically add keys to command line in the engine or utils, the current architecture (old Core.Params) allow this.
Also: modifying the command line key name and storing it in the class as string32 is more convenient to account
for both cases:
when we call the constructor with a key name with a - and without a -, so that
for example both cases:
static command_line_key<bool> mt_cdb("mt_cdb", "mt_cdb", false);
and
static command_line_key<bool> mt_cdb("-mt_cdb", "mt_cdb", false);
are valid. This will reduce the possiblity of mistakes later on.
All keys should have static lifetime and no reparsing is needed.
Key name should be pcstr.
ok, it's not hard to change then. we'll require the programmer to provide the key name with static lifetime and with no -
in the key name.
Для хранения переданных строк с ключом тоже использую указатель просто копируя соответствующий из argv. Сквашну естественно после апрува.
Excuse the ignorance, but wouldn't it be better adopting existing C++ libraries for this and reducing the amount of code in the engine itself? The more custom code that can be "outsourced" to other libraries means less maintenance work and less learning how this particular engine does it differently.
Excuse the ignorance, but wouldn't it be better adopting existing C++ libraries
That has been discussed.
Excuse the ignorance, but wouldn't it be better adopting existing C++ libraries
That has been discussed.
Could you please link me to this discussion?
Excuse the ignorance, but wouldn't it be better adopting existing C++ libraries
That has been discussed.
Could you please link me to this discussion?
Oh, it was somewhere in our Discord server and in Russian... Shortly, I didn't find any 3rd party library with the required design. The requirement was locality, the ability to declare a key as a single entity right in the file where this key is used.
Excuse the ignorance, but wouldn't it be better adopting existing C++ libraries
That has been discussed.
Could you please link me to this discussion?
Oh, it was somewhere in our Discord server and in Russian... Shortly, I didn't find any 3rd party library with the required design. The requirement was locality, the ability to declare a key as a single entity right in the file where this key is used.
Thanks. That makes sense!