iox #1067 create command line parser abstraction #1102

elfenpiff · 2022-02-14T12:58:12Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Required for the Windows command line feature in the upcoming iceoryx 2.0. This provides the building blocks to replace and unify all the command line parsing algorithms in all applications with a simple struct based interface. With this the requirement for the library getopt.h is removed and we have the same simplistic command line parsing approach on all platforms.

Furthermore, this class provides a lot of error handling, boundary checking etc. which is not present at the moment. Invalid arguments are most of the time only poorly checked and this may lead to crashes. With those classes this should be a problem of the past.

Most likely the command line parser will not become a safety certified class therefore the tests most not perform a MC/DC coverage. Nevertheless I tried to be as diligent as possible.

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
- Each unit test case has a unique UUID
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes command line parser abstraction #1067

codecov · 2022-02-14T13:22:04Z

Codecov Report

Merging #1102 (ce7f38f) into master (6a58416) will decrease coverage by 1.70%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1102      +/-   ##
==========================================
- Coverage   77.39%   75.68%   -1.71%     
==========================================
  Files         367      375       +8     
  Lines       14248    14566     +318     
  Branches     1992     2072      +80     
==========================================
- Hits        11027    11025       -2     
- Misses       2590     2909     +319     
- Partials      631      632       +1

Flag	Coverage Δ
unittests	`75.35% <0.00%> (-1.70%)`	⬇️
unittests_timing	`15.33% <0.00%> (-0.35%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...st/include/iceoryx_dust/internal/cli/arguments.inl	`0.00% <0.00%> (ø)`
...de/iceoryx_dust/internal/cli/option_definition.hpp	`0.00% <0.00%> (ø)`
...clude/iceoryx_dust/internal/cli/option_manager.inl	`0.00% <0.00%> (ø)`
iceoryx_dust/source/cli/arguments.cpp	`0.00% <0.00%> (ø)`
iceoryx_dust/source/cli/command_line_parser.cpp	`0.00% <0.00%> (ø)`
iceoryx_dust/source/cli/option.cpp	`0.00% <0.00%> (ø)`
iceoryx_dust/source/cli/option_definition.cpp	`0.00% <0.00%> (ø)`
iceoryx_dust/source/cli/option_manager.cpp	`0.00% <0.00%> (ø)`
iceoryx_hoofs/source/concurrent/loffli.cpp	`80.00% <0.00%> (-5.72%)`	⬇️

elBoberido · 2022-02-14T13:41:48Z

I can't promise to look at this anytime soon

elfenpiff · 2022-02-14T13:42:58Z

@elBoberido this is alright ... but I hoped that I maybe can bribe you with coffee, cookies, cake and chocolate.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp

FerdinandSpitzschnueffler

Only reviewed command_line.hpp and corresponding .inl and tests so far.

iceoryx_hoofs/include/iceoryx_hoofs/cxx/command_line.hpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line.inl

iceoryx_hoofs/test/moduletests/test_cxx_command_line.cpp

FerdinandSpitzschnueffler

CommandLineParser, but not the tests.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp

iceoryx_hoofs/source/cxx/command_line_parser.cpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp

iceoryx_hoofs/source/cxx/command_line_parser.cpp

CHANGELOG.md

mossmaurice

Please don't introduce new direct calls to errorHandler() in iceoryx_hoofs.

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp

FerdinandSpitzschnueffler

I checked if the CommandLineParser tests make sense; will check for missing tests in the next review round.

iceoryx_hoofs/test/moduletests/test_cxx_command_line_parser.cpp

iceoryx_hoofs/source/cxx/command_line_parser.cpp

iceoryx_hoofs/include/iceoryx_hoofs/cxx/type_traits.hpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.hpp

doc/website/release-notes/iceoryx-v1-0-0.md

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp

Signed-off-by: Christian Eltzschig <[email protected]>

…on appears Signed-off-by: Christian Eltzschig <[email protected]>

… copyright header Signed-off-by: Christian Eltzschig <[email protected]>

elBoberido

We are getting closer but still not there yet.

The tests are still no reviewed. Some fun for later.

elBoberido · 2022-07-01T12:31:28Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cli/option.hpp

+    /// @brief returns true when it contains a long option which is set
+    bool hasLongOption() const noexcept;
+
+    char shortOption = NO_SHORT_OPTION;


This somehow smells like a cxx::optional<char> shortOption or cxx::string<1> shortOption. It's not a request for change just a food for thought.

I would stick to this. The reason is that char is also somehow nullable and this I cannot avoid. What if the user explicitly state that the short option should be \0? With this approach I covered it.

Shouldn't this be a the same with a cxx::string<1> which would be empty for \0? I'm just thinking about this since it would lead to a nice symmetry in the checks with longOption. Maybe something for a future refactoring.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cli/option.hpp

elBoberido · 2022-07-01T12:44:41Z

iceoryx_hoofs/source/cli/option.cpp

+    return shortOption == '-';
+}
+
+bool Option::hasLongOptionName(const OptionName_t& value) const noexcept


Also just food for thought. matchingLongOptionName/hasMatchingLongOptionName instead of hasLongOptionName

Alright but then it would be inconsistent with the other methods. But I could rename all methods to something like matchesLongOptionName, matchesShortOptionName, matchesOptionName but I prefer the current names.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cli/option.hpp

iceoryx_hoofs/source/cli/command_line_argument_parser.cpp

elBoberido · 2022-07-01T17:18:17Z

iceoryx_hoofs/source/cli/command_line_argument_parser.cpp

+void CommandLineArgumentParser::setDefaultValuesToUnsetOptions() noexcept
+{
+    for (const auto& r : m_optionSet->m_availableOptions)
+    {
+        if (r.details.type != OptionType::OPTIONAL)
+        {
+            continue;
+        }
+
+        bool isOptionAlreadySet = false;
+        for (auto& option : m_optionValue.m_arguments)
+        {
+            if (option.isSameOption(r))
+            {
+                isOptionAlreadySet = true;
+                break;
+            }
+        }
+
+        if (!isOptionAlreadySet)
+        {
+            m_optionValue.m_arguments.emplace_back(r);
+        }
+    }
+}


This also took me a while. Actually this does not set default values to unset options since all specified options are already in m_optionValue.m_arguments. What is does is adding unspecified options to the specified options if actionWhenOptionUnknown is not UnknownOption::TERMINATE. Please keep a consistent terminology. Currently it is a mixture of unknown and unset.

I would even argue to not add the unknown options to m_optionValue.m_arguments. If the developer forgot to specify the option at least it will be revealed in tests and the incorrect behavior can be fixed.

elBoberido · 2022-07-01T17:19:55Z

iceoryx_hoofs/source/cli/command_line_argument_parser.cpp

+            }
+            case UnknownOption::IGNORE:
+            {
+                if (isNextArgumentAValue(i))


Pleas print a message that an unknown option was found and that it will be ignored.

I explicitly do not want that. Let's assume you do something like ros with the commands and subcommands and parse some arguments internally and the remaining arguments are forwarded to another command line parser which knows what to do with it.

Then it would be absolutely annoying to see messages on the console that some arguments weren't parsed by the first parser.

But what ros does with the subcommands is different that what this does, doesn't it?

Let's assume I have ./foo bar baz caramba. Then I would assume that the parser would stop parsing when the subcommand bar is found and delegate the remaining arguments to the next stage. If there is no output, how do I know that I mistyped the command like ./foo baz baz caramba?

... when thinking about it, there shouldn't be an option to ignore params but just the option to stop parsing when a subcommand is found or maybe call the nesting parser on the remaining arguments

I removed the feature again. Whenever you type an unknown option you get an error message and the help is shown.

iceoryx_hoofs/source/cli/command_line_argument_parser.cpp

elBoberido · 2022-07-03T21:42:16Z

You challenged me with the struct size ... now I throw my code in the ring :)

https://gist.github.com/elBoberido/86ec2d69912a47a5ba7cf09acba9de4e

The resulting struct has only 8 bytes of additional data ... on the plus side, the parser front-end is even simpler ... the secret is once again to prefer factories over constructors :)

…rite methods for better readability, options are only sorted before they are printed in the help, add parameter to doxygen docu Signed-off-by: Christian Eltzschig <[email protected]>

…truct by creating an explicit factory which holds all the temporary members Signed-off-by: Christian Eltzschig <[email protected]>

…e can use argv[0] system wide starting from main Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff · 2022-07-04T17:06:27Z

You challenged me with the struct size ... now I throw my code in the ring :)

https://gist.github.com/elBoberido/86ec2d69912a47a5ba7cf09acba9de4e

The resulting struct has only 8 bytes of additional data ... on the plus side, the parser front-end is even simpler ... the secret is once again to prefer factories over constructors :)

Alright you mastered the challenge! And you inspired me, I used your idea and reduced the base size of the user defined command line struct to 16 - not to 8 like you. The reason is that this command line struct stores by default the binary name which require additional 8 byte since I store the const char *.

This shouldn't pose any problem since argv[0] is valid as long as one is in main (so for the whole runtime).

…er reflected in their name Signed-off-by: Christian Eltzschig <[email protected]>

Signed-off-by: Christian Eltzschig <[email protected]>

…gumentValue Signed-off-by: Christian Eltzschig <[email protected]>

flynneva · 2022-11-04T16:19:34Z

@elfenpiff @elBoberido @dkroenke any chance we could get this merged in? Would be super useful elsewhere 😄

elBoberido · 2022-11-07T23:09:56Z

@elfenpiff @elBoberido @dkroenke any chance we could get this merged in? Would be super useful elsewhere smile

@flynneva with the introduction of iceoryx_dust we have a nice place for code which does not need to fulfill all the strict requirements for code which will be certified. @elfenpiff will continue with the effort to merge this PR

Signed-off-by: Christian Eltzschig <[email protected]>

…inted where the test ids are missing Signed-off-by: Christian Eltzschig <[email protected]>

Signed-off-by: Christian Eltzschig <[email protected]>

… case Signed-off-by: Christian Eltzschig <[email protected]>

elBoberido

Okay to be merged into dust

outdated

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from dec4497 to 10d0ea7 Compare February 14, 2022 13:02

elfenpiff requested review from elBoberido and FerdinandSpitzschnueffler February 14, 2022 13:13

elfenpiff self-assigned this Feb 14, 2022

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from 0d4c789 to 570da07 Compare February 14, 2022 13:41

dkroenke previously requested changes Feb 14, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp Outdated Show resolved Hide resolved

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from 83ca102 to 13232d8 Compare February 14, 2022 16:13

FerdinandSpitzschnueffler reviewed Feb 15, 2022

View reviewed changes

mossmaurice previously requested changes Feb 16, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/error_handling/error_handling.hpp Outdated Show resolved Hide resolved

FerdinandSpitzschnueffler reviewed Feb 16, 2022

View reviewed changes

iceoryx_hoofs/test/moduletests/test_cxx_command_line_parser.cpp Outdated Show resolved Hide resolved

FerdinandSpitzschnueffler reviewed Feb 17, 2022

View reviewed changes

iceoryx_hoofs/test/moduletests/test_cxx_command_line_parser.cpp Outdated Show resolved Hide resolved

iceoryx_hoofs/source/cxx/command_line_parser.cpp Outdated Show resolved Hide resolved

FerdinandSpitzschnueffler reviewed Feb 17, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/cxx/type_traits.hpp Show resolved Hide resolved

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.hpp Outdated Show resolved Hide resolved

dkroenke self-requested a review February 17, 2022 12:38

dkroenke added the enhancement New feature label Feb 17, 2022

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from 2e440d5 to ed271a8 Compare February 28, 2022 11:19

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from ed271a8 to d113363 Compare March 14, 2022 16:26

elfenpiff requested review from mossmaurice and FerdinandSpitzschnueffler March 14, 2022 17:12

mossmaurice reviewed Mar 15, 2022

View reviewed changes

doc/website/release-notes/iceoryx-v1-0-0.md Outdated Show resolved Hide resolved

FerdinandSpitzschnueffler reviewed Mar 16, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/command_line_parser.hpp Outdated Show resolved Hide resolved

elfenpiff force-pushed the iox-#1067-create-command-line-parser-abstraction branch from e1929cf to be68cad Compare April 5, 2022 09:05

elfenpiff added 5 commits April 5, 2022 11:20

iox-eclipse-iceoryx#1067 Moved command line parser into hoofs

1b72166

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Add command line macros option/switch macros

cc6539f

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Add missing const to call operator

2731b61

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Finalize cmd line struct usage

a042114

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Add argc offset and action when unknown opti…

58fe8ff

…on appears Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Correct method name in class diagram, update…

907a83d

… copyright header Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff requested a review from FerdinandSpitzschnueffler June 29, 2022 09:52

elBoberido reviewed Jul 1, 2022

View reviewed changes

elfenpiff added 3 commits July 4, 2022 16:12

iox-eclipse-iceoryx#1067 Restrict OptionType to uint8_t, rename & rew…

f1e0e35

…rite methods for better readability, options are only sorted before they are printed in the help, add parameter to doxygen docu Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Reduce the size of the command line parser s…

ee142a1

…truct by creating an explicit factory which holds all the temporary members Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Removed BinaryName_t to string alias since w…

d4c5973

…e can use argv[0] system wide starting from main Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff and others added 4 commits July 4, 2022 22:48

iox-eclipse-iceoryx#1067 Rename classes so that their purpose is bett…

f3f9f77

…er reflected in their name Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Update diagram svg

e2d6286

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Remove ignore unknown option feature

6203bda

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Consolidate defineOption and extractOptionAr…

54b9dcb

…gumentValue Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff added 3 commits November 8, 2022 11:18

iox-eclipse-iceoryx#1067 Merged master

c9c23ed

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Move command line parser to dust

b0e2869

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Merged master

fabbbad

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff requested a review from elBoberido November 8, 2022 11:02

elfenpiff added 5 commits November 8, 2022 12:10

iox-eclipse-iceoryx#1067 Add missing test ids

127ac36

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Adjust test id script so that the file is pr…

73141a1

…inted where the test ids are missing Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Add missing test ids

7b75c0d

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Fix clang-tidy warnings

b80119f

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1067 Function no longer nullable, remove nullable…

ce7f38f

… case Signed-off-by: Christian Eltzschig <[email protected]>

FerdinandSpitzschnueffler approved these changes Nov 17, 2022

View reviewed changes

elBoberido approved these changes Nov 17, 2022

View reviewed changes

elfenpiff mentioned this pull request Nov 17, 2022

getopt_long is not implemented in windows #1795

Open

elfenpiff merged commit dafb6fa into eclipse-iceoryx:master Nov 17, 2022

elfenpiff deleted the iox-#1067-create-command-line-parser-abstraction branch November 17, 2022 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox #1067 create command line parser abstraction #1102

iox #1067 create command line parser abstraction #1102

elfenpiff commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading

elBoberido commented Feb 14, 2022

elfenpiff commented Feb 14, 2022

FerdinandSpitzschnueffler left a comment

FerdinandSpitzschnueffler left a comment

mossmaurice left a comment

FerdinandSpitzschnueffler left a comment

elBoberido left a comment

elBoberido Jul 1, 2022

elfenpiff Jul 4, 2022

elBoberido Jul 4, 2022

elBoberido Jul 1, 2022

elfenpiff Jul 4, 2022

elBoberido Jul 1, 2022

elBoberido Jul 1, 2022

elfenpiff Jul 4, 2022

elBoberido Jul 4, 2022

elBoberido Jul 4, 2022

elfenpiff Jul 5, 2022

elBoberido commented Jul 3, 2022

elfenpiff commented Jul 4, 2022

flynneva commented Nov 4, 2022

elBoberido commented Nov 7, 2022

elBoberido left a comment

iox #1067 create command line parser abstraction #1102

iox #1067 create command line parser abstraction #1102

Conversation

elfenpiff commented Feb 14, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

elBoberido commented Feb 14, 2022

elfenpiff commented Feb 14, 2022

FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

mossmaurice left a comment

Choose a reason for hiding this comment

FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

elBoberido left a 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

elBoberido commented Jul 3, 2022

elfenpiff commented Jul 4, 2022

flynneva commented Nov 4, 2022

elBoberido commented Nov 7, 2022

elBoberido left a comment

Choose a reason for hiding this comment

elfenpiff commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading