Extract command line parser into separate file #5443
Conversation
src/command_line_parser.cc
Outdated
| } | ||
|
|
||
| // Template specialization for ParsePairOption | ||
| // [FIXME] replace ParseXXXOPtion with these |
There was a problem hiding this comment.
This feels a bit messy to read through. Could you separate the ParseXXXOption functions away from the template functions section and have a header comment for each (e.g. explaining how/when they are used, since both are being compiled into the code).
| return std::stod(arg); | ||
| } | ||
|
|
||
| // Condition here merely to avoid compilation error, this function will |
There was a problem hiding this comment.
Can you explain this further? What is the compilation error and why do we need this? If we need to keep this, ideally we need the logic included in the comment.
There was a problem hiding this comment.
it is mentioned in the second half of the comment, not the exact compilation error code though
src/command_line_parser.cc
Outdated
| #if 0 | ||
| float | ||
| ParseFloatOption(const std::string arg) | ||
| { | ||
| return std::stof(arg); | ||
| } | ||
| #endif |
| return std::stod(arg); | ||
| } | ||
|
|
||
| // Condition here merely to avoid compilation error, this function will |
There was a problem hiding this comment.
Optional if you agree, but I believe this file could be a good candidate for starting to break out some ifdefs into constexpr bools like rapids_triton does. Then all ifdefs in this source can be replaced with if (logging_enabled) { } etc., and it shouldn't complain about unused functions/variables even when these are disabled.
There was a problem hiding this comment.
This is going to be a different approach to address the problem, more on runtime check (and compiler optimization) rather than compile time selection. I am not going to do this in this PR and it will need a discussion before we agree to move towards this way.
| // [FIXME] who should call this function? | ||
| void CheckPortCollision(); |
There was a problem hiding this comment.
Is this comment still valid?
There was a problem hiding this comment.
This will probably be a helper function in main.cc, once the parser is broken down, the collision checking will be done after all endpoint parsing is done so can't be part of any option struct
No description provided.