firedancer icon indicating copy to clipboard operation
firedancer copied to clipboard

add support for cmdline bool support

Open AbhinavMir opened this issue 1 year ago • 3 comments

potential fix for https://github.com/firedancer-io/firedancer/issues/1210

AbhinavMir avatar Feb 09 '24 08:02 AbhinavMir

Thanks @AbhinavMir. Could you add tests to test_env and reformat to match the code style? You can probably also remove fd_env_strip_cmdline_contains and update usages. fd_env_strip_cmdline_bool a more powerful version of fd_env_strip_cmdline_contains.

ripatel-fd avatar Feb 15 '24 16:02 ripatel-fd

Hi @ripatel-fd , sorry I forgot to address the tests! Will do now. Also: Pasting my comments from the issues tab, I think it flew under the radar.

"What would a failure mode look like? fprintf(stderr, "Error: Invalid value for %s. Use true/false or 1/0.\n", key); sounds good?

Also, is there a make command to format it according to Firedancer's current style? I couldn't specifically find a clang formatter or such. Thx!"

AbhinavMir avatar Feb 16 '24 02:02 AbhinavMir

Hi @ripatel-fd , sorry I forgot to address the tests! Will do now. Also: Pasting my comments from the issues tab, I think it flew under the radar.

Great!

"What would a failure mode look like? fprintf(stderr, "Error: Invalid value for %s. Use true/false or 1/0.\n", key); sounds good?

Good question. fd_env is declared before fd_log, so we can't really do any error reporting. I would probably just default to "false" for now.

Although a bit verbose, we could also offer an API as such:

char const * flag_cstr = fd_env_strip_cmdline_bool( ... );
/* At this point flag_cstr contains
  1) "false" if not specified by the user, or if user explicitly specified `--flag false` or `--flag 0`
  2) "true" if specified without any argument (`--flag`), or if user explicitly specified `--flag true` or `--flag 1` 
  3) NULL if failed to parse */
if( FD_UNLIKELY( !flag_cstr )  FD_LOG_ERR(( "invalid --flag ..." ));
int flag = fd_cstr_to_bool( flag_cstr );

Also, is there a make command to format it according to Firedancer's current style? I couldn't specifically find a clang formatter or such. Thx!"

Unfortunately, we don't support an automated formatter.

This is looking pretty good now. I'm happy to do some belt-sanding for the formatting, API, and testing code. Would you be okay with me merging your code manually, and then pushing changes on top?

ripatel-fd avatar Feb 18 '24 06:02 ripatel-fd