Redis Inconsistent struct definition between commands.h and redismodule.h

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.

Comment From: enjoy-binbin

@oranagra please take a look. some PRs: - num_args was added in #10056 - the WARNING comment was added in #11051 (this PR also happens to add display_text, just above num_args)

Comment From: 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?

Comment From: 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;

Comment From: 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.

Comment From: 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;

Comment From: 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.

Comment From: enjoy-binbin

fixed in #12415