Re: Help to remove reinterpret_cast from c++ code
On Tue, 23 Apr 2013 18:15:14 CST, nvangogh <nvangogh@invalid.net>
wrote:
Hi, I have been reading a class that I found online that creates an
object to represent a joystick. Now i recall reading that any use of
'reinterpret cast' is dangerous as it is highly machine dependent. The
code I found unfortunately uses this and i was hoping to improve it,
before attempting to use the class in my program. Note, I have not
tested the class as yet - even on the assumption it works on my machine,
i need it to be as general as possible as my program will run on other
machines as well.
This is the offending member function definition:
//loop function attempts to read an event from the device, and, if an
//event is waiting to be processed,
//updates the state structure based on the event type.
void* cJoystick::loop(void *obj)
{
while (reinterpret_cast<cJoystick *>(obj)->active)
reinterpret_cast<cJoystick *>(obj)->readEv();
}
- so how can this be re-written so there are no re-interpret casts?
This is the complete class & member function definitions -if you need to
test it.
----------------------- CODE--------------------------
#ifndef JOYSTICK_H
#define JOYSTICK_H
#include <unistd.h>
class cJoystick {
public:
cJoystick();
~cJoystick();
static void* loop(void* obj);
void readEv();
joystick_position joystickPosition(int n);
bool buttonPressed(int n);
private:
pthread_t thread;
bool active;
int joystick_fd;
js_event *joystick_ev;
joystick_state *joystick_st;
__u32 version;
__u8 axes;
__u8 buttons;
char name[256];
};
/*constructor -
When a joystick is connected to a USB port, udev, the device manager for
the Linux kernel, presents
device nodes at "/dev/input/js*". constructor attempts to
1. access a device node
2. query the device for the number of buttons and axes
3. reserve space in our state structure for the buttons and axes
4. finally, create a thread to populate the state structure. */
cJoystick::cJoystick() {
active = false;
joystick_fd = 0;
joystick_ev = new js_event();
joystick_st = new joystick_state();
joystick_fd = open(JOYSTICK_DEV, O_RDONLY | O_NONBLOCK);
if (joystick_fd > 0) {
ioctl(joystick_fd, JSIOCGNAME(256), name);
ioctl(joystick_fd, JSIOCGVERSION, &version);
ioctl(joystick_fd, JSIOCGAXES, &axes);
ioctl(joystick_fd, JSIOCGBUTTONS, &buttons);
std::cout << " Name: " << name << std::endl;
std::cout << "Version: " << version << std::endl;
std::cout << " Axes: " << (int)axes << std::endl;
std::cout << "Buttons: " << (int)buttons << std::endl;
joystick_st->axis.reserve(axes);
joystick_st->button.reserve(buttons);
active = true;
pthread_create(&thread, 0, &cJoystick::loop, this);
}
}
void cJoystick::readEv() {
int bytes = read(joystick_fd, joystick_ev, sizeof(*joystick_ev));
if (bytes > 0) {
joystick_ev->type &= ~JS_EVENT_INIT;
if (joystick_ev->type & JS_EVENT_BUTTON) {
joystick_st->button[joystick_ev->number] = joystick_ev->value;
}
if (joystick_ev->type & JS_EVENT_AXIS) {
joystick_st->axis[joystick_ev->number] = joystick_ev->value;
}
}
}
// function to query the state of a button.
bool cJoystick::buttonPressed(int n) {
return n > -1 && n < buttons ? joystick_st->button[n] : 0;
}
// function to to query the state of an analog stick
joystick_position cJoystick::joystickPosition(int n) {
joystick_position pos;
if (n > -1 && n < buttons) {
int i0 = n*2, i1 = n*2+1;
float x0 = joystick_st->axis[i0]/32767.0f, y0 =
-joystick_st->axis[i1]/32767.0f;
float x = x0 * sqrt(1 - pow(y0, 2)/2.0f), y = y0 * sqrt(1 -
pow(x0, 2)/2.0f);
pos.x = x0;
pos.y = y0;
pos.theta = atan2(y, x);
pos.r = sqrt(pow(y, 2) + pow(x, 2));
} else {
pos.theta = pos.r = pos.x = pos.y = 0.0f;
}
return pos;
}
// destructor.
cJoystick::~cJoystick() {
if (joystick_fd > 0) {
active = false;
pthread_join(thread, 0);
close(joystick_fd);
}
delete joystick_st;
delete joystick_ev;
joystick_fd = 0;
}
#endif
Not easily. The problem is that you're passing an untyped parameter
through the thread creation call, which leaves you somewhat stuck. The
thread itself (::loop), just gets the untyped pointer to the object,
you'll have to do something to convert it back to a pointer of the
proper type.
You could pass it in a static area, but that's a bad idea for the
usual reasons.
But this should be portable to any system supporting pthreads (all the
Posix stuff you have in there is a different issue, of course).
Pthreads requires the ability to convert void pointers to general
pointers as is being done.
FWIW, the Windows threading API passes the thread parameter exactly
the same way.
And actually it's perfectly defined in C and C++ to convert a pointer
to a void pointer, and then back to a pointer of the *same* type
(which is what you're doing here). Note that some other threading
APIs use something other than a void pointer as the parameter to the
thread, and some people put a value other than a pointer into the void
* parameter, both of which are more problematic from a C/C++ standard
point of view. And you don't need a reinterpret cast for that, a
static cast is just fine.
But for clarity, I'd change the code a bit to something like:
static void* cJoystick::loop(void *obj)
{
cJoystick *jp = static_cast<cJoystick *>(obj);
while (jp->active)
jp->readEv();
}
And my personal preference is to actually put the mainline of the
thread into a proper non-static member function, and have just a
static stub function to handle the thread startup, which leaves you
with something like:
static void*
cJoystick::static_function_specified_in_pthread_create(void *obj)
{
cJoystick *jp = static_cast<cJoystick *>(obj);
jp->loop();
}
void* cJoystick::loop() //not static!
{
while (active)
readEv();
}
Alternatively some threading libraries (like boost:thread) can hide
the thread creation and parameter passing for you, although that may
be overkill for this.
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]