Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix option parser's long option handling #355

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

cniethammer
Copy link
Contributor

Long option names were only compared to known options based on the length of the option name given as arguments, resulting in strange matches, e.g., '--step' was matching '--steps'.

This resolves the parsing issue in #351

Long option names were only compared to known options based on the length
of the option name given in the arguments, resulting in strange matches,
e.g., '--step' was matching '--steps'.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer cniethammer added the bug Something isn't working label Dec 6, 2024
@cniethammer cniethammer requested a review from HomesGH December 6, 2024 21:15
Copy link
Contributor

@HomesGH HomesGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix for this part of the issue. Do you also want to fix the other part of the issue with this PR or a separate one?
Additionally:
If no valid option is passed, a segmentation fault occurs. This is due to the order of initialization in MarDyn.cpp. First, the Logger should be inited, then the options parsed etc. However, this might not be trivial as the init of the Logger depends on an option. We could init with the default (i.e. print to stdout) and later switch to another Logger if the respective option is specified. What do you think?

ls1-mardyn/src/MarDyn.cpp

Lines 143 to 157 in 98597e0

optparse::OptionParser op;
initOptions(&op);
optparse::Values options = op.parse_args(argc, argv);
std::vector<std::string> args = op.args();
/* Initialize the global log file */
if( options.is_set_by_user("logfile") ) {
// Print to file
std::string logfileNamePrefix(options.get("logfile"));
std::cout << "Using logfile with prefix " << logfileNamePrefix << std::endl;
Log::global_log = std::make_unique<Log::Logger>(Log::Info, logfileNamePrefix);
} else {
// Print to stream (default: std::cout)
Log::global_log = std::make_unique<Log::Logger>(Log::Info);
}

@cniethammer
Copy link
Contributor Author

I would stay with just fixing the addressed part of incorrect argument handling with this PR.

I will open a separate PR with a fix for the initialisation problem - as this is a different topic.

@HomesGH HomesGH self-requested a review December 11, 2024 12:36
@cniethammer cniethammer merged commit 55cccdf into ls1mardyn:master Dec 11, 2024
52 checks passed
@cniethammer cniethammer deleted the fix-option-parser branch December 11, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants