Comment On Please Think Twice

Don't think twice, it's alright

I've done some similar coding early on in my career because it seemed like a good idea at the time, but I always put in a few words as to what I was trying to accomplish and why...

"no, no, really, it's the other rjacobs"

Please think twice before reading this post.

Ok, so wtf is the point of this code?

You shouldn't leave your computer logged in and all that when you go get your bowl of ramen, man.

Captcha: sino, just like the ramen.

The beauty of C is that you can write such code when you really need it.

Note that it is considered more elegant to use an union to store a float into a char array.

So what's a better way of storing the binary representation of a float into a byte array?

And thus begins the long road to a schizophrenic breakdown.

I would say that this is pretty typical. Most coders think their code is soooo good that anyone else (read lesser mortal) would need to think twice to improve their code. BTW, the emoticon is priceless.

Please think twice before trying to improve on this comment...

:^p

I was asked to write code like that once to put integers into a linux kernel driver without using IOCTL.

Funny after we got it working that way we ended up using IOCTL instead anyway.

I like doing stuff like this:

// DO NOT CHANGE! VERY IMPORTANT!

private static readonly int magicNumber = 62892;

Where, of course, the actual value is unimportant.

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

In an application I was maintaining I found the well-intentioned statement:

buffer[strlen(buffer)] = '\0';

amischiefr:

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

Honest people?

Mark V Shaney:

The beauty of C is that you can write such code when you really need it.

Note that it is considered more elegant to use an union to store a float into a char array.

At this very moment, I'm refactoring a block of floats into unions because a very late new requirement (6 years out-of-date!) forces me to jam a load of int32s into a structure I cannot extend, on pain of... pain. I already have a magic 'convert anything 32-bit to anything else' union. I just love embedded C on a small processor !

JS:

amischiefr:

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

Honest people?

Those are few and far between these days... :-(

In an application I was maintaining I found the well-intentioned statement:

buffer[strlen(buffer)] = '\0';

argh!

Fred:

So what's a better way of storing the binary representation of a float into a byte array?

*((float*)(&x[4])) = value;
maybe?

This would NEVER happen to me! NEVER, I TELL YA!

Thief^:

Fred:

So what's a better way of storing the binary representation of a float into a byte array?

*((float*)(&x[4])) = value;
maybe?

memcpy(&x[4], &value, sizeof value);

This should also save you from some crashes due to unaligned access.

SenTree:

Mark V Shaney:

The beauty of C is that you can write such code when you really need it.

Note that it is considered more elegant to use an union to store a float into a char array.

At this very moment, I'm refactoring a block of floats into unions because a very late new requirement (6 years out-of-date!) forces me to jam a load of int32s into a structure I cannot extend, on pain of... pain. I already have a magic 'convert anything 32-bit to anything else' union. I just love embedded C on a small processor !

Suggestion: To save space, only use 31 bits for every int32 (offset every following int32 by -1 bit). This way you can stuff another int32 every 31st int32's. The 32nd bit is very rarely used anyway...

amischiefr:

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

Someone who has learned the errors of his past ways.

He hopes to atone for his WTF sins by repenting and sharing with us. We can mock him, then he will forgiven and saved from Robot Hell. (Thats where WTF coders go)

TheRider:

Suggestion: To save space, only use 31 bits for every int32 (offset every following int32 by -1 bit). This way you can stuff another int32 every 31st int32's. The 32nd bit is very rarely used anyway...

Only for the SIGN ! (or did you mean the other end ?). Good laugh, though.

Seriously, it's not the space that's the problem. We have a distributed multi-processor system with some associated PC software, all of which relies on the communications packets and configuration records not changing. I have to implement a backwards-compatible fix, otherwise I have to rewrite a hell of a lot more code - and deploy it in the field. Since the 'field' includes Siberian coalmines, and we have no remote update system, and I'm the only person qualified to do it, that ain't gonna happen.

Mark V Shaney:

The beauty of C is that you can write such code when you really need it.

The horror of C is that it allows you to just say "It may be ugly, but I really need it" rather than spend effort and find a better way (which may involve refactoring).

brazzy:

Mark V Shaney:

The beauty of C is that you can write such code when you really need it.

The horror of C is that it is really ugly.

Fixed.

Really though, I don't like ampersands everywhere.

Implicit unions are MUCH cooler:







#include <math.h>



template <class A, class B> union union_pair

{

A first;

B second;

union_pair(const A &init) : first(init) { }

union_pair(const B &init) : second(init) { }

union_pair &operator=(const A &val) { first = val; return *this; }

union_pair &operator=(const B &val) { second = val; return *this; }

operator const A &() { return first; }

operator const B &() { return second; }

};

typedef union_pair<int, float> intfloat;



float InvSqrt(intfloat x)

{

float save = x;

x = 0x5f3759df - (x >> 1);

return 1.5f * pow(x, 1) - 0.5f * save * pow(x, 3);

}

OperatorBastardusInfernalis:

Implicit unions are MUCH cooler:







#include <math.h>



template <class A, class B> union union_pair

{

A first;

B second;

union_pair(const A &init) : first(init) { }

union_pair(const B &init) : second(init) { }

union_pair &operator=(const A &val) { first = val; return *this; }

union_pair &operator=(const B &val) { second = val; return *this; }

operator const A &() { return first; }

operator const B &() { return second; }

};

typedef union_pair<int, float> intfloat;



float InvSqrt(intfloat x)

{

float save = x;

x = 0x5f3759df - (x >> 1);

return 1.5f * pow(x, 1) - 0.5f * save * pow(x, 3);

}

For you, Sir, there is most certainly a special place in hell. ;)

Because its funny.

I've done that before. I needed to send a float over a network connection, so I used something like that to pack it into a 32-bit int. GCC complains with the warning "dereferencing type-punned pointer breaks strict aliasing rules". I ignored it, since it normally works, but when I put the code in an inline function and turned optimization on, it stopped working. The proper way is to use a union:

inline uint32_t pack_float(float f) {

    union {

        float flt;

        uint32_t intg;

    } convert;

    convert.flt = f;

    return convert.intg;

}

No pesky warnings, and with optimization, it compiles to the same code.

Anrs:

Thief^:

Fred:

So what's a better way of storing the binary representation of a float into a byte array?

*((float*)(&x[4])) = value;
maybe?

memcpy(&x[4], &value, sizeof value);

This should also save you from some crashes due to unaligned access.

Err, how do you know that x needs to be 4 bytes?

Technically, you should malloc x to the sizeof value.

Also you should take the address of x, not the address of x[4].

so, something like:
char *x = (char*) malloc(sizeof(value));
memcpy(&x,&value, sizeof(value));

Is that right? I haven't done C for about 6 year.

P.S. I HATE C

if you are converting to a char, why the hell would you leave it in binary in the first place?! Isn't that just a bunch of wasted bytes? I mean, why send this over the wire:

"1111"

When you could just do this:

"15"

Or this:

"F"

Unless, of course, you don't own both ends of the pipe, and the stupid end requires a binary char array instead of hex...

amischiefr:

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

Someone secure in their personhood.

Actually, I've found admitting my own WTFs to management generally results in a lessened impact, especially if I've come up with a fix before I approach management about it. For years, I kept the group I was in from having actual SLA requirements by generally meeting the service levels provided by other groups, and giving honest accounts of what went wrong along with plans to fix them, whenever we did have a problem.

One might think that this wouldn't be that useful a feat, since it required one to actually maintain good service. However, it allowed us several liberties:

1. If we wanted to do a change that wouldn't impact many people, we could do it any time of day - no change window needed.

2. We generally took our service outages in significant, unplanned chunks (planned service was generally performed with little to no outage). By announcing who dun it and what they were doing to fix it before most people knew about the problem, people generally didn't have time to get angry at an anonymous entity, so had nothing to transfer when they did find out. And, in general, people don't get angry as quickly at known persons with good reputations as they do at anonymous entities.

3. We didn't have to take the time to negotiate SLAs.

4. We didn't have to collect metrics to show we met our SLAs.

5. We didn't have to attend the quarterly SLA meeting.

6. We didn't get dinged for failing to meet our SLAs. (Note: we did get dinged for our various oopses - but groups with SLAs also got dinged for their various oopses; the SLA ding was in addition to those.)

7. When we finally did get an SLA (seems we got to the point where we were the only Corporate group without an SLA, and someone (VIDepartment) missed their SLA due to one of our outages - not that our outage was that long, but they were on the borderline), we were given significantly more latitude in setting our SLA.

Of course, not having an SLA did have one disadvantage:

1. We didn't get the bonus associated with meeting the SLA longer than anyone ever met their SLA, that we probably wouldn't have gotten anyway, because we wouldn't have met our SLA for that long.

John:

Anrs:

memcpy(&x[4], &value, sizeof value);

Err, how do you know that x needs to be 4 bytes?

Technically, you should malloc x to the sizeof value.

Also you should take the address of x, not the address of x[4].

so, something like:
char *x = (char*) malloc(sizeof(value));
memcpy(&x,&value, sizeof(value));

Is that right? I haven't done C for about 6 year.

Your snippet is wrong, but more importantly, your objection is wrong, too. The &x[4] is the destination in the existing char array, it has nothing to do with the size of a float.

bex:

if you are converting to a char, why the hell would you leave it in binary in the first place?! Isn't that just a bunch of wasted bytes?

You miss the point entirely. Maybe "compacted binary" would make more sense to you.

To put it in more specific terms, we're talking about a float (4 byte floating point) being stored as a 4-character string.

Generally, this is done for storage reasons, as the normal output routines for floating point convert them to a human-understandable string representation. One does not want to store the string "0.34832923" when one can just store 4 characters - especially since storing the string could introduce a rounding error.

jspenguin:

I've done that before. I needed to send a float over a network connection, so I used something like that to pack it into a 32-bit int. GCC complains with the warning "dereferencing type-punned pointer breaks strict aliasing rules". I ignored it, since it normally works, but when I put the code in an inline function and turned optimization on, it stopped working. The proper way is to use a union:

inline uint32_t pack_float(float f) {

    union {

        float flt;

        uint32_t intg;

    } convert;

    convert.flt = f;

    return convert.intg;

}

No pesky warnings, and with optimization, it compiles to the same code.

And it wont work unless you always use it to transmit data between the same platforms. Consider what happens when endianness or floating point formats differ ... The correct solution would be to use XDR or ASN.1.

Alternative way would have been to give -fno-strict-aliasing option to gcc (I've done it with some older code written before gcc learned to broke^Woptimise such casts). Using uint32_t is good idea as the unsigned int is not 4 byte on all platforms.

Portable and readable solution would be of course to use memcpy. ;)

It's rev1 to rev3. it was probably an import. Not everybody used vcs from the beginning of the project..

My C pointers are really rusty, but would not that code only work if a float takes up the same number of bits of memory as a float?

and, aren't floats normally 64 bits wide, and ints normally 32bits?

fatgeekuk:

My C pointers are really rusty, but would not that code only work if a float takes up the same number of bits of memory as a float?

A float always takes up the same number of bits of memory as a float (sorry, couldn't resist) ;/

fatgeekuk:

and, aren't floats normally 64 bits wide, and ints normally 32bits?

Only on systems where that's the case. Many embedded systems have 32 bit floats and 16 bit ints - in fact I can think of one which has 8 bit ints and (I think) 24 bit floats.

JS:

amischiefr:

Now, while we all make mistakes and sometimes they are pretty stupid I have to ask one questions: who the hell in their right mind posts their own WTF's?

Honest people?

People always lie.

(Sorry been watching the House DVDs recently)

fatgeekuk:

My C pointers are really rusty, but would not that code only work if a float takes up the same number of bits of memory as a float?

and, aren't floats normally 64 bits wide, and ints normally 32bits?

hmmm, just checked.... I was wrong (at least I am maintaining todays average)

floats are 4bytes long. bugger.

I always thought.... well, never mind.

Gieron:

I like doing stuff like this:

// DO NOT CHANGE! VERY IMPORTANT!

private static readonly int magicNumber = 62892;

Where, of course, the actual value is unimportant.

In an application I maintain, which includes config/environment variables all starting with the same prefix, and all capital letters, I know some people run "strings" on new releases, looking for "hidden" config variables.

I've been tempted to add strings to the program which look like a config variable of the client's name (ie: "XYZ_CLIENT"), just to see if they'd say anything. I never had the nerve to actually try it, however.

troels:

In an application I was maintaining I found the well-intentioned statement:

buffer[strlen(buffer)] = '\0';

argh!

I use "argv" and "argc" all the time. What's "argh"?

Of course, on "Talk Like a Pirate Day", it's "aaaarrrghv".

I'd like to point out that of the various methods given in the reactions, Anrs method is one of the best:

    void store(char *a, float f) {

        memcpy(&a, &f, sizeof(float));

    }

It has the big advantage over other methods given here, that it works for unaligned byte arrays. If you ever programmed for a CPU that disallows unaligned memory access (Windows CE/PocketPC is a common example) you should know how annoying it is when library code ported from the x86 crashes on unportable hacks like this.

If you think the solution using memcpy() is too slow, look at the code your compiler generates. All modern compilers contain special optimizations for common functions like memcpy(). The call above will actually be eliminated by the compiler and replaced by the efficient single assignment on platforms where it is safe to do so. So in practice, you get good performance, without sacrificing portability.

The downside of the memcpy() method is that it stores bytes in whatever order the local platform uses, which means that the resulting data isn't portable across platforms even if they use the same (IEEE) float representation.

To work around that, jspenguin's union trick helps,
although you still have to manually store the data in a character array:

    void store(unsigned char *a, float f) {

        uint32_t i = convert_float_to_uint(f);

        /* Store in little endian byte order */

        a[0] = i&255; i >>= 8;

        a[1] = i&255; i >>= 8;

        a[2] = i&255; i >>= 8;

        a[3] = i&255; i >>= 8;

    }

java.lang.Chris;:

jspenguin:

I've done that before. I needed to send a float over a network connection, so I used something like that to pack it into a 32-bit int. GCC complains with the warning "dereferencing type-punned pointer breaks strict aliasing rules". I ignored it, since it normally works, but when I put the code in an inline function and turned optimization on, it stopped working. The proper way is to use a union:

inline uint32_t pack_float(float f) {

    union {

        float flt;

        uint32_t intg;

    } convert;

    convert.flt = f;

    return convert.intg;

}

No pesky warnings, and with optimization, it compiles to the same code.

And it wont work unless you always use it to transmit data between the same platforms. Consider what happens when endianness or floating point formats differ ... The correct solution would be to use XDR or ASN.1.

I forgot to add that I converted the resulting uint32_t to network byte order. So, the de facto protocol spec says, "A IEEE 754 single precision floating point number in network byte order". Almost *every* processor with an FPU uses IEEE 754. On processors that don't have an FPU, the floating point emulation that GCC links in uses IEEE 754.

This protocol is a real-time protocol for games -- strictly UDP. Using XDR would be overkill for this.

Hmmm.

"Whoops."

Yeah did that myself once. Bitched about some lousy code in a project that had been done years before by contractors.

Turned out I was one of those contractors.

Whoops!

memcpy(x+4, &value, sizeof(value));

To put it in more specific terms, we're talking about a float (4 byte floating point) being stored as a 4-character string.

ahh... makes more sense now. Thanks for the clarification.