Generate optstrings automatically #864
Conversation
This commit introduces a new function construct_optstring that constructs the optstring for getopt_long based on the long_options array. We used to manually edit the optstring in each command's getopts function. This was error-prone and tedious. This commit removes the manual editing and replaces it with a call to construct_optstring.
There was a problem hiding this comment.
It is nice that static arrays can be used that way to generate the opt string, but now we are doing the operation at run-time instead of compile-time. Is there a way that we could do that operation at compile-time?
I know C is a poor platform for adding compile-time tooling, but there might be a clean way to still do that?
…rings-automatically
I do not think that we can do that in C. If you think that it is not reasonable to do this operation in run-time, we can close this pull request. IMO, it is risky to assume that people will do a good job maintaining these strings by hand, and it is a meaningful compromise.
In recent versions of C++, there are several ways that usually include variadic templates, and/or cryptic class definitions. I failed to find an elegant solution in C++, and not any solution in C. I tried to implement macros that concatenate characters at compile time, but that does not work with variables, only string constants. |
| @@ -653,8 +681,7 @@ cli_copy_db_getopts(int argc, char **argv) | |||
| exit(EXIT_CODE_BAD_ARGS); | |||
| } | |||
|
|
|||
| const char *optstring = | |||
| "S:T:D:J:I:b:L:u:mcAPOXj:xBeMlUagkynF:F:Q:irRCN:fp:ws:o:tE:Vvdzqh"; | |||
There was a problem hiding this comment.
If we generate the optstring value for clone command at run-time, we get "S:T:D:J:J:I:b:L:L:u:mcAPOXj:xBBeMMlUagkynF:F:Q:irRCN:fp:ws:o:tE:Vvvdzqh".
This character array contains duplicate characters due to the aliases that we have, and works fine.
Another thing that I noticed is that we had a substring F:F: that also repeated a short option string.
There was a problem hiding this comment.
Is there a way to remove duplicates in the code that builds the string here? I am worried about portability of duplicates depending on getopt_long implementations.
There was a problem hiding this comment.
I added deduplication logic. However to make things simple, I assumed the aliases will always be consecutive in the long_options array.
There was a problem hiding this comment.
Maybe we could use our hash lib to handle that fully, and hide that behind a DEBUG flag so that we don't do the dedup operation at run-time in “production” builds?
There was a problem hiding this comment.
Good idea. I think I can take this idea and build some more on top of it to come up with a nice design.
I will rewrite the helper function in a way that:
- on prod: it will immediately return supplied static strings
- on debug builds: run dedup logic, and verify that the result is identical to the supplied static string. If they are not identical, it will result in an assertion failure that should guide the developer into how they should update the string.
This way, we will be able to catch potential bugs in the future as CI will catch those assertions
| @@ -107,7 +107,9 @@ cli_dump_schema_getopts(int argc, char **argv) | |||
| exit(EXIT_CODE_BAD_ARGS); | |||
| } | |||
|
|
|||
| while ((c = getopt_long(argc, argv, "S:T:D:PrReFCNVvdzqh", | |||
There was a problem hiding this comment.
We should have N: as snapshot has a required argument.
{ "snapshot", required_argument, NULL, 'N' }
I wonder if this means we never supported pgcopydb dump schema --snapshot <snapshot_id> properly.
There was a problem hiding this comment.
I see. Can we include a fix for the argument parsing in this PR then?
There was a problem hiding this comment.
Once we programmatically create the optstring values, it will fix this underlying issue as well.
This bug only exists in the line that I want to remove.
| @@ -294,7 +294,7 @@ cli_list_db_getopts(int argc, char **argv) | |||
| exit(EXIT_CODE_BAD_ARGS); | |||
| } | |||
|
|
|||
| const char *optstring = "S:D:s:t:F:xPL:u:k:mfyarJRIN:Vdzvqh"; | |||
There was a problem hiding this comment.
We should not have k: as it has no arg. { "skip-split-by-ctid", no_argument, NULL, 'k' }
There was a problem hiding this comment.
That is something that this PR is fixing, right?
There was a problem hiding this comment.
Yes. I should have made myself clear.
I wanted to document the human errors when maintaining optstring values by hand. This bug will not be possible after we start generating the optstring values programmatically.
If we have multiple aliases for the same option, we should only include the first one in the optstring. To make deduplication easy, we assume aliases are consecutive in the long_options array.
…rings-automatically
... and replace the hardcoded optstring with a dynamically generated one
This commit introduces a new function construct_optstring that constructs the optstring for getopt_long based on the long_options array.
We used to manually edit the optstring in each command's getopts function. This was error-prone and tedious. This commit removes the manual editing and replaces it with a call to construct_optstring.