shared/options: add an assert loop to verify ordering

If things are misordered, we need to catch this. Use assert_se to make
the check also in custom builds that otherwise disable assertions.

diff --git a/src/shared/options.c b/src/shared/options.c
index 364b57792c8778c093cdafcdda9928f707c0d979..227ba8ffa352227bf281b67f78e139f893b9df4e 100644 (file)
--- a/src/shared/options.c
+++ b/src/shared/options.c
@@ -101,6 +101,12 @@ int option_parse(
                         state->namespace_start = options;
 
                 const Option *opt;
+
+                /* Verify that the option array didn't get mangled within a namespace. */
+                for (opt = options; opt < options_end; opt++)
+                        if (opt + 1 < options_end && !FLAGS_SET((opt + 1)->flags, OPTION_NAMESPACE_MARKER))
+                                assert_se(opt->id < (opt + 1)->id);
+
                 for (opt = options; opt < options_end; opt++) {
                         bool ns_marker = FLAGS_SET(opt->flags, OPTION_NAMESPACE_MARKER);
                         if (!in_ns) {

diff --git a/src/shared/options.h b/src/shared/options.h
index 8b8fc9ed4a81951b230c1e12143882562ad78267..7bb73dd5811c2edbdb627f8d7de305284a3a879e 100644 (file)
--- a/src/shared/options.h
+++ b/src/shared/options.h
@@ -10,7 +10,14 @@
  * and in a single (unnamed) group. OPTION_NAMESPACE() marks the beginning of a named namespace.
  * OPTION_GROUP() marks the beginning of a named group.
  *
- * Note: if multiple namespaces are used, they should all be named.
+ * Note: if multiple namespaces are used, they should all be named, i.e. each separate parse_argv
+ * instance should have OPTION_NAMESPACE first, and then its set of OPTION()s. (This is because
+ * clang reorders OPTIONs coming from different functions. So an unnamed group could end up being
+ * merged with one of the earlier groups. It seems that reordering within a single function does
+ * not happen.)
+ *
+ * When groups are used, the first group may be named (with OPTION_GROUP appearing before any
+ * options), or it may be unnamed. Both variants should work fine.
  */
 
 typedef enum OptionFlags {

diff --git a/src/test/test-options.c b/src/test/test-options.c
index a77964df4e1fa40234a6454167cf7e280331f3f2..150c2b2a5664a068fdf42aad1bf89f1b3e69b389 100644 (file)
--- a/src/test/test-options.c
+++ b/src/test/test-options.c
@@ -631,12 +631,12 @@ TEST(option_stops_parsing) {
 
 TEST(option_group_marker) {
         static const Option options[] = {
-                { 1, .short_code = 'h', .long_code = "help" },
-                { 2, .long_code = "version" },
-                { 0, .long_code = "AdvancedGroup", .flags = OPTION_GROUP_MARKER },
-                { 3, .long_code = "debug" },
-                { 4, .long_code = "Advance" },  /* prefix match with the group */
-                { 5, .long_code = "defilbrilate" },
+                { __COUNTER__, .short_code = 'h', .long_code = "help" },
+                { __COUNTER__, .long_code = "version" },
+                { __COUNTER__, .long_code = "AdvancedGroup", .flags = OPTION_GROUP_MARKER },
+                { __COUNTER__, .long_code = "debug" },
+                { __COUNTER__, .long_code = "Advance" },  /* prefix match with the group */
+                { __COUNTER__, .long_code = "defilbrilate" },
                 {}
         };
 
@@ -1297,9 +1297,9 @@ TEST(option_macros) {
  * peeks at the next arg to handle legacy "space-separated" form. */
 TEST(option_optional_arg_consume) {
         static const Option options[] = {
-                { 1, .short_code = 'h', .long_code = "help" },
-                { 2, .long_code = "user", .metavar = "NAME", .flags = OPTION_OPTIONAL_ARG },
-                { 3, .short_code = 'u', .long_code = "uid", .metavar = "USER" },
+                { __COUNTER__, .short_code = 'h', .long_code = "help" },
+                { __COUNTER__, .long_code = "user", .metavar = "NAME", .flags = OPTION_OPTIONAL_ARG },
+                { __COUNTER__, .short_code = 'u', .long_code = "uid", .metavar = "USER" },
                 {}
         };