Trying to call a function pointer causes a compile error with GNUG++

[Alex Buell]

Hi,

I've got this class that parses command line input, which won't
compile, the error message is:

% g++ buggy.cpp -o buggy
buggy.cpp: In member function ‘int Command::execute()’:
buggy.cpp:110: error: must use ‘.*’ or ‘->*’ to call pointer-to-member
function in ‘((Command*)this)->Command::cmd.
std::_Rb_tree_iterator<_Tp>:perator-> [with _Tp = std:air<const
Command::ELEMENT, int>]()->std:air<const Command::ELEMENT,
int>::first.Command::ELEMENT::function (...)’

And the code itself is as follows (I've simply boiled it down to a
short 124 line program) I'd be grateful if you can point out what I'm
doing wrong when calling via Command::execute(), many thanks!

#include <iostream>
#include <map>
#include <vector>
#include <string>
#include <cassert>

class Command
{
public:
Command(std::vector<std::string>& arguments);
~Command();

int execute();

struct ELEMENT
{
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);
}

friend std:stream& operator<<(std:stream& 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;
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 }
};

std:stream& operator<<(std:stream& os, const Command::ELEMENT& rhs)
{
os << rhs.command;
return os;
}

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

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++;
}
}
}

Command::~Command()
{
delete commands;
}

int Command::a(void)
{
std::cout << "a\n";
}

int Command::b(void)
{
std::cout << "b\n";
}

int Command::c(void)
{
std::cout << "c\n";
}

int Command::execute()
{
if (cmd->first.function != NULL)
return (cmd->first.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);

Command command(arguments);
command.execute();
}

 

[Rolf Magnus]

And the code itself is as follows (I've simply boiled it down to a
short 124 line program) I'd be grateful if you can point out what I'm
doing wrong when calling via Command::execute(), many thanks!

Well, the error tells you exactly what you did wrong and how to fix it:

buggy.cpp:110: error: must use ‘.*’ or ‘->*’ to call pointer-to-member

Instead of

return (cmd->first.function)();

write

return (cmd->first.*function)();


Btw: It could have been helpful to mark line 110, so people here don't need
to count 110 lines to find out which line the error message belongs to (ok, I
decided to count backwards, since you told us the total number of lines, so
it was only 14 lines to count).

 

[Vidar Hasfjord]

Well, the error tells you exactly what you did wrong and how to fix it:


Instead of


write

 return (cmd->first.*function)();

No, that is erroneous as well. Here 'function' is a member of 'first'
which is of type ELEMENT. The whole expression '(cmd->first.function)'
correctly refers to the member pointer. What is missing is an object
on which to invoke the function. If the intention is to invoke it on
the current object (*this), then correct statement is:

return (this->*(cmd->first.function)) ();

or in more readable form:

typedef int (Command::*Fun) ();
Fun f = cmd->first.function;
return (this->*f) (); // invoke on this

NB. The original posted code doesn't compile. The function
"strcasecmp" is not declared, and functions 'a', 'b' and 'c' should
return a value.

Regards,
Vidar Hasfjord

 

[Rolf Magnus]

No, that is erroneous as well. Here 'function' is a member of 'first'
which is of type ELEMENT. The whole expression '(cmd->first.function)'
correctly refers to the member pointer. What is missing is an object
on which to invoke the function.

Of course, you are right. I did answer too quickly withouth looking at the
code closely enough. My bad.

 

[Alex Buell]

Well, the error tells you exactly what you did wrong and how to fix
it:


Instead of


write

return (cmd->first.*function)();

That's exactly what I thought would work. It did not.

 

[Alex Buell]

No, that is erroneous as well. Here 'function' is a member of 'first'
which is of type ELEMENT. The whole expression '(cmd->first.function)'
correctly refers to the member pointer. What is missing is an object
on which to invoke the function. If the intention is to invoke it on
the current object (*this), then correct statement is:

return (this->*(cmd->first.function)) ();

or in more readable form:

typedef int (Command::*Fun) ();
Fun f = cmd->first.function;
return (this->*f) (); // invoke on this

NB. The original posted code doesn't compile. The function
"strcasecmp" is not declared, and functions 'a', 'b' and 'c' should
return a value.

The code that I post except for the compile error that I found,
compiles perfectly under GNU G++ 4.1.2. With your helpful fix for the
problem I mentioned, it now compiles. I hadn't realised you needed an
object to invoke the function call with.

A big thanks to all involved.

 

[Vaclav Haisman]

Alex Buell wrote, On 25.12.2008 12:34:

Hi,

I've got this class that parses command line input, which won't
compile, the error message is:

% g++ buggy.cpp -o buggy
buggy.cpp: In member function ‘int Command::execute()’:
buggy.cpp:110: error: must use ‘.*’ or ‘->*’ to call pointer-to-member
function in ‘((Command*)this)->Command::cmd.
std::_Rb_tree_iterator<_Tp>:perator-> [with _Tp = std:air<const
Command::ELEMENT, int>]()->std:air<const Command::ELEMENT,
int>::first.Command::ELEMENT::function (...)’

The mesage pretty much says it all. You need to use one of the two constructs
to dereference member function pointer.

And the code itself is as follows (I've simply boiled it down to a
short 124 line program) I'd be grateful if you can point out what I'm
doing wrong when calling via Command::execute(), many thanks!

#include <iostream>
#include <map>
#include <vector>
#include <string>
#include <cassert>

class Command
{
public:
Command(std::vector<std::string>& arguments);
~Command();

int execute();

struct ELEMENT
{
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);
}

friend std:stream& operator<<(std:stream& os, const ELEMENT& rhs);

std::string command;
int (Command::*function)();

Member function pointer is different than free function or static class
function pointer.

};

private:
int a(void);
int b(void);
int c(void);

static const int COMMANDS;
static const ELEMENT command_table[];

std::map<ELEMENT, int>* commands;
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 }
};
[...]

int Command::execute()
{
if (cmd->first.function != NULL)
return (cmd->first.function)();

This should read:
return (cmd->*first.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);

Command command(arguments);
command.execute();
}

 

[Alex Buell]

This should read:
return (cmd->*first.function)();

Thanks, but the correct solution, as pointed out previously is:

return (this->*(cmd->first.function))();

Regards,
Alex

 

[Thomas J. Gritzan]

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:stream& operator<<(std:stream& 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:stream& operator<<(std:stream& os, const Command::ELEMENT& rhs)
{
os << rhs.command;
return os;
}

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

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);

You don't have to push_back the arguments. Prefer initialization:

 

[Alex Buell]

You alread have the solution, but I wanted to add some additional
points.

Thanks, that's even more welcome than the solution!

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.

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.

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:stream& operator<<(std:stream& 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:stream& operator<<(std:stream& os, const Command::ELEMENT&
rhs) {
os << rhs.command;
return os;
}

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

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);

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!

 

[Rolf Magnus]

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.

If you don't want to support copying the object, simply declare a copy
constructor and assignment operator as private, and don't implement them.
That'll do the trick. If an accidental attempt to copy the object slips in,
you'll get an error from the compiler or from the linker.

std:stream& operator<<(std:stream& os, const Command::ELEMENT&
rhs) {
os << rhs.command;
return os;
}

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

You can use std::make_pair here.

What's the difference between that and what I've used?

You don't need the typedef. You can just write:

commands->insert(std::make_pair(command_table, i));

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.

The point is that commands->end() is an iterator to one past the end, i.e.
to a non-existing element. Therefore, cmd->first is invalid in that case. You
also have to check that cmd != commands->end().

 

[Alex Buell]

If you don't want to support copying the object, simply declare a copy
constructor and assignment operator as private, and don't implement
them. That'll do the trick. If an accidental attempt to copy the
object slips in, you'll get an error from the compiler or from the
linker.

Right, I see.

std:stream& operator<<(std:stream& os, const
Command::ELEMENT& rhs) {
os << rhs.command;
return os;
}

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

You can use std::make_pair here.

What's the difference between that and what I've used?

You don't need the typedef. You can just write:

commands->insert(std::make_pair(command_table, i));

I think that does read a lot better than the other approach.

The point is that commands->end() is an iterator to one past the end,
i.e. to a non-existing element. Therefore, cmd->first is invalid in
that case. You also have to check that cmd != commands->end().

OK, all that's needed is the check != commands->end(). Thanks.

 

[Thomas J. Gritzan]

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.

You did delete it, but the copy and assignment weren't there.
Forbidding copy and assignment would work, of course.

Command::Command(std::vector<std::string>& arguments)
{
typedef std:air<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));

You can use std::make_pair here.

What's the difference between that and what I've used?

As Rolf Magnus wrote, you don't need the typedef. I wondered what you
need the typedef for as I looked over the code.

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.

Provide a correct operator<.

I don't know if strcasecmp provides a strikt weak ordering needed for
std::sort and std::map, but std::lexicographic_compare with toupper()
should do it. As fifth parameter to lexicographic_compare, pass a
function that returns toupper(left) < toupper(right).

When that is done, you would have to instantiate an ELEMENT with the
command you want to find and pass it to find(). If find() returns end(),
its not in the map.

In this case, unless you need the struct for something else, I would
drop the ELEMENT struct and use a
std::map<std::string, function_pointer_type, LessComparator>,
where LessComparator would be a function that uses
std::lexicographic_compare internally. You could pass the command you
want to find directly to find().

If the map won't change after initialization, you could even make it a
std::vector, sort it (with LessComparator above), and use
std::binary_search (with same comparator). This saves you all the
overhead of std::map and has better cache locality.

 

[Alex Buell]

If the map won't change after initialization, you could even make it a
std::vector, sort it (with LessComparator above), and use
std::binary_search (with same comparator). This saves you all the
overhead of std::map and has better cache locality.

If I was to do something like:

std::vector<std::string, int (*function)(), std::string> commands;

Would this work with what I've got in mind?

i.e conduct a search with the first parameter in the vector?

Thanks

 

[Thomas J. Gritzan]

If I was to do something like:

std::vector<std::string, int (*function)(), std::string> commands;

Would this work with what I've got in mind?

i.e conduct a search with the first parameter in the vector?

No. std::vector can only store one type of object. You need a struct to
combine the string(s) with the function pointer.