Fix issue #240: Multiple long option names / aliases #349

eyalroz · 2022-07-09T18:53:19Z

Edit 2: This PR is finalized, now with some testing

We now use a vector of long option names instead of a single name
When specifying an option, you can provide multiple names separated by commas, at most one of which may have a length of 1 (not necessarily the first specified name). The length-1 name is the single-hyphen switch (the "short name").
Hashing uses the first long name
Option help currently only uses the first long name. Not sure if that should change.
Utilizing this feature in unit tests in test/options.cpp and also in src/example.cpp.

This change is

jarro2783 · 2022-07-11T07:09:42Z

This looks like a good approach. Given the way short and long names are basically an alias already, I think this can mostly be reused as you have done to just make more aliases.

include/cxxopts.hpp

* We now use a vector of long option names instead of a single name * When specifying an option, you can provide multiple names separated by commas, at most one of which may have a length of 1 (not necessarily the first specified name). The length-1 name is the single-hyphen switch (the "short name"). * Hashing uses the first long name * Option help currently only uses the first long name. Not sure if that should change. * Almost no tests for this functionality yet * Using `std::token_regex_iterator` to tokenize for option-splitting

jarro2783 · 2022-07-12T20:56:18Z

include/cxxopts.hpp

-  std::match_results<const char*> result;
-  std::regex_match(text.c_str(), result, option_specifier);
-  if (result.empty())
+  if (not std::regex_match(text.c_str(), option_specifier))


can you use ! instead of not

I could... but - although personally I've long switched to only using and, or and not to make my code more natural-language like.

jarro2783 · 2022-07-12T21:00:03Z

include/cxxopts.hpp


-  return std::pair<std::string, std::string>(short_sw, long_sw);
+  constexpr const int use_non_matches { -1 };


I think the extra const is redundant.

@jarro2783 : Only in C++11. If you compile with C++14 or later, const isn't implied. I don't mind very much, the only reason I have this is to avoid using -1 as a "magic number".

Ok I didn't realise that, this is fine then.

Actually I think you've misread the statement about const.

A constexpr specifier used in an object declaration [or non-static member function (until C++14)] implies const

It is only non-static member functions that were dropped from implying const. So this extra const is redundant.

Well, that's counter-intuitive, but ok. Dropped the const.

…t/options.cpp`.

jarro2783 · 2022-07-12T21:09:30Z

include/cxxopts.hpp

+  OptionNames all_option_names = values::parser_tool::split_option_names(opts);
+    // Note: All names will be non-empty!
+  std::string short_sw {""};
+  OptionNames long_option_names = all_option_names;


do you need this extra copy? It looks like you don't use all_option_names again.

I don't. I just wanted to clarify this is no longer all_option_names. Ideally, a compiler could optimize this away; but I suppose I could use std::move. On the other hand, there are soooo many string copies in cxxopts - it could really stand to use string_view everywhere.

jarro2783 · 2022-07-12T21:18:36Z

include/cxxopts.hpp

+      throw_or_mimic<exceptions::invalid_option_format>(opts);
+    }
+    short_sw = *first_length_1_name_it;
+    long_option_names.erase(first_length_1_name_it);


This seems a bit complicated. Are you basically trying to find one short name and then leave multiple long names?
I wonder if a simple loop would just be clearer. Or maybe you could use remove_if and then check if the remaining list is greater than 1.

Are you basically trying to find one short name and then leave multiple long names?

Yes.

I wonder if a simple loop would just be clearer.

I try to follow Sean Parent's No Raw Loops principle...

Or maybe you could use remove_if and then check if the remaining list is greater than 1.

But then I would need to re-obtain the options I've erased.

Do you think an explanatory comment would help?

e.g. after I assign long_option_names, I could say:

// currently, this may still hold one or more single-char (=short) names; let's take care of those

alternatively, I could use std::partition and check how many short options we have. What do you think?

Well remove_if doesn't actually remove it, it only moves everything to the end, so I think this is what you want.

Oh I just read that the values at the end have unspecified value, so they might get destroyed. In that case partition is probably the way to go.

Have done this with std::partition, have a look at my force-pushed change.

jarro2783

Overall looking pretty good, just a few nitpick comments.

jarro2783 · 2022-07-13T21:39:59Z

This is good, thanks for the effort. Can you just update that const line, and can you also add an entry into the changelog?

eyalroz · 2022-07-13T22:41:49Z

@jarro2783 : I already have updated the const line... and am now force-pushing a CHANGELOG line addition.

* Added a `CHANGELOG.md` entry about the multiple-long-names feature. * Simplified the separation of the short name from the long names using `std::partition` * Using C-style logical operators: `!`, `&&`, `||` rather than `not`, `and`, `or` * `std::move()`ing from a vector of strings instead of just copying it. * `constexpr const` -> just `constexpr`, since that implies const, weirdly enough.

Related to jarro2783#240, jarro2783#363, and jarro2783#349

eyalroz force-pushed the fix-240 branch 3 times, most recently from 978c27d to 3b763c6 Compare July 9, 2022 21:20

jarro2783 reviewed Jul 11, 2022

View reviewed changes

include/cxxopts.hpp Outdated Show resolved Hide resolved

eyalroz mentioned this pull request Jul 11, 2022

Fixes #345, fixes #346: Exception code tweaks #347

Merged

eyalroz force-pushed the fix-240 branch 2 times, most recently from 1f2279d to a2c4b7f Compare July 12, 2022 18:51

eyalroz changed the title ~~WIP on issue #240: Multiple long option names / aliases~~ Fix issue #240: Multiple long option names / aliases Jul 12, 2022

eyalroz force-pushed the fix-240 branch from a2c4b7f to 5cfc740 Compare July 12, 2022 20:50

eyalroz mentioned this pull request Jul 12, 2022

Regards #340, #345: More unindentation #351

Merged

jarro2783 reviewed Jul 12, 2022

View reviewed changes

Added some test code for jarro2783#240 to existing unit tests in `tes…

d994b9b

…t/options.cpp`.

jarro2783 reviewed Jul 12, 2022

View reviewed changes

eyalroz force-pushed the fix-240 branch from 5a2a600 to 312d5ee Compare July 12, 2022 22:04

eyalroz force-pushed the fix-240 branch from 312d5ee to 90725ed Compare July 13, 2022 21:40

eyalroz force-pushed the fix-240 branch from 90725ed to 8e525e2 Compare July 13, 2022 22:43

jarro2783 merged commit e976f96 into jarro2783:master Jul 14, 2022

eyalroz mentioned this pull request Aug 13, 2022

Add more alias for command-line options eyalroz/gpu-kernel-runner#48

Closed

amirsojoodi added a commit to amirsojoodi/cxxopts that referenced this pull request Nov 30, 2023

Print long names in help option

dfeeffc

Related to jarro2783#240, jarro2783#363, and jarro2783#349

This was referenced Nov 30, 2023

Print long names in help option #412

Open

Print long names in help option amirsojoodi/cxxopts#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #240: Multiple long option names / aliases #349

Fix issue #240: Multiple long option names / aliases #349

eyalroz commented Jul 9, 2022 •

edited

Loading

jarro2783 commented Jul 11, 2022

jarro2783 Jul 12, 2022

eyalroz Jul 12, 2022 •

edited

Loading

jarro2783 Jul 12, 2022

eyalroz Jul 12, 2022

jarro2783 Jul 12, 2022

jarro2783 Jul 13, 2022

eyalroz Jul 13, 2022

jarro2783 Jul 12, 2022

eyalroz Jul 12, 2022

jarro2783 Jul 12, 2022

eyalroz Jul 12, 2022

eyalroz Jul 12, 2022

jarro2783 Jul 12, 2022

jarro2783 Jul 12, 2022

eyalroz Jul 12, 2022 •

edited

Loading

jarro2783 left a comment

jarro2783 commented Jul 13, 2022

eyalroz commented Jul 13, 2022 •

edited

Loading


		return std::pair<std::string, std::string>(short_sw, long_sw);
		constexpr const int use_non_matches { -1 };

Fix issue #240: Multiple long option names / aliases #349

Fix issue #240: Multiple long option names / aliases #349

Conversation

eyalroz commented Jul 9, 2022 • edited Loading

jarro2783 commented Jul 11, 2022

Choose a reason for hiding this comment

eyalroz Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalroz Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

jarro2783 left a comment

Choose a reason for hiding this comment

jarro2783 commented Jul 13, 2022

eyalroz commented Jul 13, 2022 • edited Loading

eyalroz commented Jul 9, 2022 •

edited

Loading

eyalroz Jul 12, 2022 •

edited

Loading

eyalroz Jul 12, 2022 •

edited

Loading

eyalroz commented Jul 13, 2022 •

edited

Loading