Re: Trying to call a function pointer causes a compile error with
GNU G++
On Fri, 26 Dec 2008 01:20:19 +0100, I waved a wand and this message
magically appears in front of Thomas J. Gritzan:
You alread have the solution, but I wanted to add some additional
points.
Thanks, that's even more welcome than the solution!
#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".
Since the class is only going to be used once, and it's not going to be
copied or assigned, maybe I can explicitly define these with = 0 to
forbid these operations.
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.
I'll change that to something else in lower case at some point in time.
{
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.
Right, I see.
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's odd, I'm definitely sure I did put code to delete the pointer in
the destructor. I use valgrind to find these sort of memory leaks.
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];
Now, that's a much better solution, I was wondering the exact same thing
when I wrote that code but I wasn't aware that it was possible to work
out the size of the table!
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.
What's the difference between that and what I've used?
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.
I don't know enough yet to write a find() funtion for the ELEMENT struct
that takes a std::string and returns the appropriate element that the
std::map class can use.
}
[...]
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.
Is there an alternative? I use NULL in the struct ELEMENT's command
member to indicate that the command doesn't have a callable function.
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);
That one, I've implemented. It does look a lot more elegant!
--
http://www.munted.org.uk
Fearsome grindings.