C - should I use a variable array of function pointers

Question

This question is about how to solve my problem on the level of how I design my program. For a school project, I'm building a shell, which has several built-in functions. One of these function's purpose (cmd_type) is to check to see if the argument provided is in that list of functions. Here is a partial implementation of it:

int cmd_type(int argc, char *argv[]) {
    if (argc == 2) {
        for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
            if (strcmp(cmds_name[i], argv[1]) == 0) {
                printf("%s is a shell builtin\n", argv[1]);
                return 0; // found it
            }
        }

        // still need to search path, call stat(path/cmd)
        errmsg("not implemented! type", 1);
    } else {
        err_msg("type", 1);
    }
}

Defining manual if statements for every function my shell supports sounds like a bad choice because the list might expand over time, and I need to store the list of function names anyway. So originally, I planned to define an array of function names and an array of their pointers, like so:

char cmds_name[BUILTIN_FUNC_COUNT-1][16];
char (*cmds_ptr)(int,*char[])[BUILTIN_FUNC_COUNT-1];
// make list of built-in funcs
strcpy(cmds_name[0], "exit");
strcpy(cmds_name[1], "cd");
// make list of func pointers
cmds_ptr[0] = &cmd_exit;
cmds_ptr[1] = &cmd_cd;

They're accessed like so:

// try builtin cmds
for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
    if (strcmp(cmds_name[i], argv[0]) == 0) {
        last_cmd_err = (*cmds_ptr[i])(argc, argv);
        continue; // we found it, so next loop
    }
}

Then they'd each happily take (int argc, char *argv[]) as arguments. But the cmd_path() needs access to the list in addition to those arguments, so I'd have to define it as a global, or define a global pointer to it... In the process of researching this, I found this answer, saying a similar approach was really bad style: https://stackoverflow.com/a/41425477/5537652

So my questions are: Is this a good way to solve this problem, or should I just do if/else statements/is there a better way? Would you recommend a global pointer to the array of function names?

One way around it is to define an array of structures (rather than two independent arrays of names and pointers), and to define the functions with a prototype of int builtin_cmd(int argc, char **argv, void *extra);. The extra pointer points to whatever extra information the function needs. It would be better if you can devise a type — a structure pointer of some sort, probably — rather than the wishy-washy void *, but that is the most general type. The functions that don't need extra information can be passed a null pointer, or can ignore the pointer that they are passed. — Jonathan Leffler
– Jonathan Leffler, Commented Jul 9, 2017 at 2:12
Before globalizing or hiding, make a structure with name and a function pointer a pair structure and use a binary search by sorting them in an array by name. — BLUEPIXY
– BLUEPIXY, Commented Jul 9, 2017 at 2:39
OT: This char (*cmds_ptr)(int,*char[])[BUILTIN_FUNC_COUNT-1]; does not correspond very well with this for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) { if (strcmp(cmds_name[i], .... — alk
– alk, Commented Jul 9, 2017 at 12:07

Zakir · Accepted Answer · 2017-07-09 03:09:40Z

I am going to propose a structure of cmd_name and function pointer like this:

typedef struct{
  char cmds_name[16];
  char (*cmds_ptr)(int,*char[]);
} cmd_type;

Now define a static table of this type for all your cmds:

static const cmd_type cmd_table[] = {
  {"exit", &cmd_exit},
  {"cd", &cmd_cd},
  .......
  .......
};

Finally access it like this:

for (int i = 0; i < BUILTIN_FUNC_COUNT; i++) {
  if (strcmp(cmd_table[i].cmds_name, argv[0]) == 0) {
    last_cmd_err = (*cmd_table[i].cmds_ptr)(argc, argv);
    continue; // we found it, so next loop
  }
}

The decision to choose between if-else vs a global table is a matter of personal taste and coding style. I would prefer the above solution simply because it improves ** code readability** and reduces clutter. There may be other constraints in your environment that can influence your decision - like if the no of table entries is huge and there is a limitation on global memory space - the if-else route would be a better choice..

HTH!

sg7 · Accepted Answer · 2017-07-09 03:17:10Z

I would not go with if-else statements. There is nothing wrong with solution (2) proposed in https://stackoverflow.com/a/41425477/5537652.

You could have a table with a string and a function to service an entry:

typedef struct cmd_desc
{
  char cmd[80];
  int builtin_cmd(int argc, char **argv, void *extra);
} CMD_DESC;

static CMD_DESC descTable[] =
{
  { "exit",                 cmd_exit      },      
  { "cd",                   cmd_cd        },   
  { "$ON_OPEN_CMD",         OnOpenCmd     },
  { "$OPEN_EXTRA_CMD",      OpenExtraCmd  },
  { "$AC",                  ActionCmd     },
  { "$AD",                  ActionDataCmd },
  { "$EC",                  ExtraCmd      },
  { "$TC",                  TextCmd       },
  { "",                     NULL          }
};

int cmd_exit (int argc, char **argv, void *extra)
{
  //...
}

Access/execution:

for (int tokenIndex=0; strcmp(descTable[tokenIndex].cmd,""); tokenIndex++) //search table 
{
    if ( strcmp( (descTable[tokenIndex]).cmd, argv[0] ) == 0 )
    { 
        int ret = (*(descTable[tokenIndex]).builtin_cmd( argc, argv, extra);
    }
}

I used the above approach in a my applications and it worked well for me.

The table can be easily expanded and the readability of the table is better than if/else chain.

Collectives™ on Stack Overflow

C - should I use a variable array of function pointers

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related