Inconsistent struct definition between commands.h and redismodule.h

[georgeanderson]

Which one below is telling the truth?

commands.h:

/* WARNING! This struct must match RedisModuleCommandArg */
typedef struct redisCommandArg {
    const char *name;
    redisCommandArgType type;
    int key_spec_index;
    const char *token;
    const char *summary;
    const char *since;
    int flags;
    const char *deprecated_since;
    int num_args;
    struct redisCommandArg *subargs;
    const char *display_text;
} redisCommandArg;

redismodule.h:

typedef struct RedisModuleCommandArg {
    const char *name;
    RedisModuleCommandArgType type;
    int key_spec_index;       /* If type is KEY, this is a zero-based index of
                               * the key_spec in the command. For other types,
                               * you may specify -1. */
    const char *token;        /* If type is PURE_TOKEN, this is the token. */
    const char *summary;
    const char *since;
    int flags;                /* The REDISMODULE_CMD_ARG_* macros. */
    const char *deprecated_since;
    struct RedisModuleCommandArg *subargs;
    const char *display_text;
} RedisModuleCommandArg;

Notice int num_args; is missing from redismodule.h.

[enjoy-binbin]

@oranagra please take a look.
some PRs:

• num_args was added in Move doc metadata from COMMAND to COMMAND DOCS #10056
• the WARNING comment was added in Repurpose redisCommandArg's name as the unique ID #11051 (this PR also happens to add display_text, just above num_args)

[oranagra]

i think that warning (WARNING! This struct must match RedisModuleCommandArg) is excessive.
we can't update the struct in redismodule.h without breaking ABI, and in fact, there's no need.
this field is computed at runtime, and these two structs don't have to be the same:

        cmd->args = moduleCopyCommandArgs(info->args, version);
        /* Populate arg.num_args with the number of subargs, recursively */
        cmd->num_args = populateArgsStructure(cmd->args);

maybe instead of warning that they should be the same, we should give a reference to moduleCopyCommandArgs in that comment?
@guybe7 am i missing anything?

[georgeanderson]

How about an inline comment like this?

@@ -29,7 +29,7 @@ typedef struct redisCommandArg {
     const char *since;
     int flags;
     const char *deprecated_since;
-    int num_args;
+    int num_args; /* computed at runtime, no need to sync with RedisModuleCommandArg */
     struct redisCommandArg *subargs;
     const char *display_text;
 } redisCommandArg;

[oranagra]

it seems good, but if we'll have other runtime fields it can become messy.
also, i still think that it's a good idea to provide a reference to moduleCopyCommandArgs.

[georgeanderson]

Sure, here is another attempt. What do you think?

@@ -19,7 +19,8 @@ typedef enum {
 #define CMD_ARG_MULTIPLE        (1<<1)
 #define CMD_ARG_MULTIPLE_TOKEN  (1<<2)
 
-/* WARNING! This struct must match RedisModuleCommandArg */
+/* Must be compatible with RedisModuleCommandArg. Make use of moduleCopyCommandArgs
+ * for converting between the two. */
 typedef struct redisCommandArg {
     const char *name;
     redisCommandArgType type;

[oranagra]

LGTM, maybe "make use of" is inaccurate (converts in just one direction, and we don't have to encourage people to do that conversion).
maybe just See moduleCopyCommandArgs.
feel free to make a PR.

[enjoy-binbin]

fixed in #12415