Help to remove reinterpret_cast from c++ code

[nvangogh]

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

[Mathias Gaunard]

Post by nvangogh
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?

Use static_cast instead.

[red floyd]

Post by nvangogh
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.
//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();
}

Use static_cast<>

[Seungbeom Kim]

Post by nvangogh
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.

[...]

Post by nvangogh
void* cJoystick::loop(void *obj)
{
while (reinterpret_cast<cJoystick *>(obj)->active)
reinterpret_cast<cJoystick *>(obj)->readEv();
}

[...]

Post by nvangogh
pthread_create(&thread, 0, &cJoystick::loop, this);

[...]

The value of 'this' will be static_cast'ed into void*, so I see no
problem in cJoystick::loop static_cast'ing obj back into cJoystick*.

(I guess that, in case void* and cJoystick* have different layouts,
static_cast does the right thing but reinterpret_cast may not.)

void* cJoystick::loop(void *arg)
{
cJoystick* obj = static_cast<cJoystick*>(arg); // do it only once
while (obj->active)
obj->readEv();
}

or, you can define another non-static member function that executes
the loop, and call it from cJoystick::loop.

inline void cJoystick::readEvWhileActive()
{
while (active) readEv();
}

void* cJoystick::loop(void *arg)
{
static_cast<cJoystick*>(arg)->readEvWhileActive();
}

[Daniel Krügler]

Post by Seungbeom Kim
The value of 'this' will be static_cast'ed into void*, so I see no
problem in cJoystick::loop static_cast'ing obj back into cJoystick*.

Agreed.

Post by Seungbeom Kim
(I guess that, in case void* and cJoystick* have different layouts,
static_cast does the right thing but reinterpret_cast may not.)

Correct according to the wording, but existing implementations already
did "the right thing" independent of these constraints. During the C++
meeting in Bristol last week, CWG issue

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1412

became accepted, so the reinterpret_cast is now equivalent to the
static_cast in this case.

Greetings from Bremen,

Daniel Krügler

[Daniel Krügler]

Post by nvangogh
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.
//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();
}

What does this function return?

Post by nvangogh
- so how can this be re-written so there are no re-interpret casts?

Technically you could rewrite this as:

void* cJoystick::loop(void* obj)
{
cJoystick& pJ = *static_cast<cJoystick*>(obj);
while (pJ.active)
pJ.readEv();
return nullptr; // Or whatever is right here
}

I find this more readable, but it still has the basically the same
meaning as the alternative version with the reinterpret_cast (modulo
some minor details that I think are not relevant in the context of
your question) and also cannot validate what you are doing here.

I assume you wrote this function with opaque parameters, because you
have to adapt to some low-level API. Instead of investing much efforts
to replace the reinterpret_cast here by static_cast, I recommend to
invest time to design a public API that does not depend on such
low-level details, instead wrapping this part into a layer that has
full control over the input and output of such functions. std::thread
is a good example for this: It completely hides the OS layer that
typically has exactly similar internal APIs as your cJoystick class.

HTH & Greetings from Bremen,

Daniel Krügler

[Andy Champ]

Post by nvangogh
so how can this be re-written so there are no re-interpret casts?

This code actually looks safe to me - it's one of the cases where the
cast is doing exactly what you need. And the reason why you need it is
down to the interface into the OS not being templated :)

This is what MS say on the subject: "The result of a reinterpret_cast
cannot safely be used for anything other than being cast back to its
original type. Other uses are, at best, nonportable.". Your use is one
of the safe ones.

Your call is effectively this line:

pthread_create(&thread, 0, &cJoystick::loop, this);

This creates a thread, passing it a pointer to the static function
cJoystick::loop (which must have a signature "void name(void*)) and a
pointer to the data item that gets passed into it. In your code the
pointer is a pointer to a cJoystick, and is automatically cast to
void*.

The OS code then calls your supplied static function, passing it the
void pointer to your parameter - which is always a cJoystick.

If your parameter wasn't a cJoyStick it would most probably crash.

As a side issue (and it most likely will make no difference to the
compiled code) I'd write the routine like this. It gives you a place
you can stop the debugger, and look at your cJoystick object.

void* cJoystick::loop(void *obj)
{
cJoystick* pThis = reinterpret_cast<cJoystick *>(obj);

while (pThis->active)
{
pThis->readEv();
}
}

Andy

[Ulrich Eckhardt]

Post by nvangogh
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.

Yes, use of reinterpret_cast always has a code smell.
[...]

Post by nvangogh
void* cJoystick::loop(void *obj)
{
while (reinterpret_cast<cJoystick *>(obj)->active)
reinterpret_cast<cJoystick *>(obj)->readEv();
}
pthread_create(&thread, 0, &cJoystick::loop, this);

Ah, and it is even used wrongly. The conversion from "this" to "void*"
is equivalent to a static_cast. In the loop function, it should do
this:

cJoystick* js = static_cast<cJoystick*>(obj);
while(...)
...


Of course, with modern C++ there's std::thread and thus no reason to
use the raw POSIX thread interface at all. Doing so would hide this
cast.
That said, the code smells in many other aspects:
- exception safety
- Law of Three
- not checking returnvalues/errorcodes
- not synchronizing access to resources shared between threads
- use of nonstandard __u8 types that should be uint8_t
- C-style casts
- public functions loop() and readEv() should be private
- const correctness of buttonPressed()


Uli

[Francis Glassborow]

Post by nvangogh
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.
//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 code looks very dangerous. As it stands the function accepts any
pointer regardless and then uses it as a cJoystick *. In essence you
cannot fix it without determining why the author wrote code that
requires the user to ensure that what they pass to cJoystick::loop can
be safely treated as a cJoystick *. My reaction is that the problem lies
with the parameter. It also lacks a return statement though the
function declares a return type.

I think I would rewrite the function as:

cJoystick * cJoystick::loop(cJoystick *obj){
while (obj)->active) (obj)->readEv();
return obj;
}

I am not enamoured with the parameter name (which seems to consider a
pointer as being a cJoystick object)


Now when you make that change you will probably get lots of errors
because you will have to ensure that what is passed to the function is a
suitable pointer (i,e, a pointer to an object that can legitimately be
used as a cJoystick.

I am also a little curious as to why a member function is taking a
pointer that it then forces into being a pointer to the type of the class.

I have to say that code like that does not look very promising. I looked
quickly at the rest of the code and note that the above function is a
static function, so an object of type cJoystick does not even have to exist.

Looks to me like either you use the code as is (and have no hope that it
will run on any platform other than the one it is written for, or you
spend time understanding the requirements and rewrite the code entirely.

Francis

[Robert Wessel]

Post by nvangogh
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.
//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 {
cJoystick();
~cJoystick();
static void* loop(void* obj);
void readEv();
joystick_position joystickPosition(int n);
bool buttonPressed(int n);
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.

[Ike Naar]

Post by nvangogh
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.
//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?

You could use static_cast instead of reinterpret_cast.
By the way, your function does not return a value (according to its
prototype, it should return a pointer to void).

[Gerhard Fiedler]

Post by nvangogh
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.
//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 should be a static_cast here. It also becomes more readable if you
write it a bit differently (mostly for longer functions, but the
principle is always useful):

void* cJoystick::loop(void *obj)
{
cJoystick *joystick = static_cast<cJoystic *>(obj);
while (joystick->active)
joystick->readEv();
}

The use of a cast in this situation (pthread thread functions) is
normal, but it should be a static_cast.

Gerhard