Re: Trying to call a function pointer causes a compile error with
GNU G++
Alex Buell wrote:
Hi,
I've got this class that parses command line input, which won't
compile, the error message is:
You alread have the solution, but I wanted to add some additional points.
#include <iostream>
#include <map>
#include <vector>
#include <string>
#include <cassert>
class Command
{
public:
Command(std::vector<std::string>& arguments);
~Command();
You have a destructor, but no copy constructor and no copy assignment
operator. If you have one, you usually need all three.
Google for "c++ rule of three".
int execute();
struct ELEMENT
In C and C++, identifiers with ALL CAPS usually are reserved for macros.
This rule is to avoid name clashes. Experienced programmers expect
ELEMENT to be a macro.
{
public:
bool operator==(const ELEMENT& rhs) const
{
return (strcasecmp(command.c_str(), rhs.command.c_str()) == 0);
}
bool operator<(const ELEMENT& rhs) const
{
return (strcasecmp(command.c_str(), rhs.command.c_str()) != 0);
}
This is not a valid operator< if you want to use it for sorting or a
std::map. Anyway, strcasecmp is not defined, you need to include a
header for it. It's not a C or C++ function, it's in the POSIX standard.
friend std::ostream& operator<<(std::ostream& os, const ELEMENT& rhs);
std::string command;
int (Command::*function)();
};
private:
int a(void);
int b(void);
int c(void);
static const int COMMANDS;
static const ELEMENT command_table[];
std::map<ELEMENT, int>* commands;
If you have a pointer in your class and allocate memory for it, you need
to free it in the destructor, and you need to define a copy constructor
and a copy assignment operator.
That leads to the question: Why is commands a pointer actually?
Make it a regular member.
std::map<ELEMENT, int>::iterator cmd;
};
const int Command::COMMANDS = 3;
const Command::ELEMENT Command::command_table[Command::COMMANDS] =
{
{ "a", &Command::a },
{ "b", &Command::b },
{ "c", &Command::c }
};
Why if you add or remove an element to the table and forget to adjust
the int above?
It might be better to remove the COMMANDS variable and calculate the
size of the table if you need to:
sizeof command_table / sizeof command_table[0];
std::ostream& operator<<(std::ostream& os, const Command::ELEMENT& rhs)
{
os << rhs.command;
return os;
}
Command::Command(std::vector<std::string>& arguments)
{
typedef std::pair<ELEMENT, int> command;
commands = new std::map<ELEMENT, int>;
cmd = commands->end(); // make sure it's set to the end
for (int i = 0; i < COMMANDS; i++)
commands->insert(command(command_table[i], i));
You can use std::make_pair here.
assert((int)commands->size() == COMMANDS);
if (arguments.size() > 0)
{
cmd = commands->begin();
while (cmd != commands->end())
{
if (strcasecmp(cmd->first.command.c_str(), arguments[0].c_str()) == 0)
{
arguments.erase(arguments.begin());
break; // We've got the command
}
cmd++;
}
What's the reason you have a std::map and don't use it for lookup? If
you search for the command string incrementally, you could use a
std::vector instead, or drop the container and search in the statically
declared command_table.
}
[...]
int Command::execute()
{
if (cmd->first.function != NULL)
return (cmd->first.function)();
cmd is set to end() in the constructor. You must not dereference this
iterator, if it still is end(). Checking the function pointer for NULL
does not help.
return -1;
}
int main(int argc, char* argv[])
{
std::vector<std::string> arguments;
for (int i = 1; i < argc; i++)
arguments.push_back(argv[i]);
You don't have to push_back the arguments. Prefer initialization:
std::vector<std::string> arguments(argv, argv+argc);
Command command(arguments);
command.execute();
}
--
Thomas