Re: problem with cast and unions

torri wrote:
On Thu, 20 Aug 2009 14:19:40 +0100, Ben Bacarisse wrote:

Yes, but I don't see why you would try to hide the problem in a macro.

as i said, i didn' write the code. Someone else thought it was easier.

Remember, just getting it to compile won't necessarily get it to work, even if you have done something that seems sensible to solve each problem.

I'd write the example as:

static Embryo_Cell
_embryo_fp(Embryo_Program *ep __UNUSED__, Embryo_Cell *params) {
/* params[1] = long value to convert to a float */
Embryo_Float_Cell efc;
if (params[0] != (1 * sizeof(Embryo_Cell))) return 0; efc.f =

This looks highly suspicious to me. It looks like the program uses the size of a structure to determine what type of structure it is. I'm guessing here that when you go far enough up the call tree you will reach sizeof(Ebryo_Cell) being passed sometimes, and sizeof(something_else) being passed on other occasions. If I'm write, then depending on what all the something_else's could be, and on padding, type sizes etc, you could sometimes get false positives.

I'll bet there are other functions that do different things as well.

(float)params[1];

The cast is not required, and personally I would drop it.

return efc.c;

OK, so here it is trying to reinterpret the bit patted of a float as an int.

}

good enough for me, even if i don't have a macro.

Not for me, you could have all sorts of other problems...

If you can't re-write to avoid all this messing about, you can convert
via a pointer:

#define EMBRYO_FLOAT_TO_CELL(f) (((Embryo_Float_Cell *)&(f))->c)

but there is a lot of finger-crossing going on in all these cases.

it's too ugly. I'll do the clean way, switching between types I want.

There is NO clean way to do it, since you (or rather the original author) is trying to do something dirty. It is also something that is not guaranteed to work for all sorts of reasons...

Firstly, int and float might not be the same size. You need to check this, and work out what to do if they are different sizes. If they are the same size you don't have to do anything about it for *this* port.

Secondly, writing to one member of a union then reading from another is not guaranteed to work.

Looking at the code, if I'm reading it write...
You are receiving an int as a parameter, converting it to a float (correctly) and then returning the bit pattern of that float interpretted as an int. I'm guessing that various parts of the software pass ints around, and at some point the bit pattern will be re-interpretted as a float to get something useful.

I was going to suggest something that might be cleaner, but then I realised there were too many things going on for my idea to work without an extra copy. Instead I'll suggest...

static Embryo_Cell
_embryo_fp(Embryo_Program *ep __UNUSED__, Embryo_Cell *params)
{
float f;
assert(sizeof(Embrio_Cell) == sizeof(float));
/* or use one of the compile-time assert methods which have been suggested here, such as
typedef int compile_time_assert[(sizeof(Embrio_Cell) == sizeof(float))?1:-1]; */

/* params[1] = long value to convert to a float */

if (params[0] != (1 * sizeof(Embryo_Cell))) return 0;
f = params[1];
return *(Embryo_Cell*)f;
}


Still ugly as hell, but removes one of the freedoms the compiler has to "break" this code.

I may well have miss-read the code, since it was so horrible (to me) I had to read it with my eyes closed...
--
Flash Gordon
.

Quantcast