struct Vector4
{
Vector4(float _x, float _y, float _z, float _w)
: x(_x), y(_y), z(_z), w(_w) {}
Vector4(float _x, float _y, float _z)
: x(_x), y(_y), z(_z), w(1.0f) {}
Vector4(float _x, float _y)
: x(_x), y(_y), z(0.0f), w(1.0f) {}
float x, y, z, w;
};
// Then, somewhere...
unsigned int color = 0xFF00AA00; // could be anything, doesn't matter
Vector4 vColor(
((float) ((color & 0x00FF0000) >> 16)) / 255.0f, // R
((float) ((color & 0x0000FF00) >> 8)) / 255.0f, // G
((float) ((color & 0x000000FF)) / 255.0f, // B
((float) ((color & 0xFF000000) >> 24)) / 255.0f)); // A
Try to find the bug :)
Author
Hey!
A little challenge for you guys [smile]
Enjoy :)
AH... it is a parenthesis error. You're calling the second overloaded constructor
Vector4(float _x, float _y, float _z)
because the last two values are grouped in one pair of parentheses.
Vector4(float _x, float _y, float _z)
because the last two values are grouped in one pair of parentheses.
Vector4 vColor(
((float) ((color & 0x00FF0000) >> 16)) / 255.0f, // R
((float) ((color & 0x0000FF00) >> 8)) / 255.0f, // G
((float) ((color & 0x000000FF)) / 255.0f, // B
((float) ((color & 0xFF000000) >> 24)) / 255.0f)); // A
((float) ((color & 0x00FF0000) >> 16)) / 255.0f, // R
((float) ((color & 0x0000FF00) >> 8)) / 255.0f, // G
((float) ((color & 0x000000FF)) / 255.0f, // B
((float) ((color & 0xFF000000) >> 24)) / 255.0f)); // A
It helps if you line up the brackets in such statements, like this:
Vector4 vColor( ((float) ((color & 0x00FF0000) >> 16)) / 255.0f, // R ((float) ((color & 0x0000FF00) >> 8)) / 255.0f, // G ((float) ((color & 0x000000FF) )) / 255.0f, // B ((float) ((color & 0xFF000000) >> 24)) / 255.0f // A );Quote:The explicit cast to float is redundant, too. Just cluttering up the code IMO.
No its not, implicit conversions from integral types to floating point types generate a warning when using the highest warning level on most compilers and since you are compiling your code with treat warnings as errors on the highest warning level the cast is necessary to compile and serves as documentation of the intent of the programmer.
It should, however, use C++ style casts which gets rid of a lot of the brackets making things more readable.
Vector4 vColor( static_cast<float>((color & 0x00FF0000) >> 16) / 255.0f // R , static_cast<float>((color & 0x0000FF00) >> 8) / 255.0f // G , static_cast<float>((color & 0x000000FF) ) / 255.0f // B , static_cast<float>((color & 0xFF000000) >> 24) / 255.0f // A );Compilers don't tend to warn about implicit casts to types with a higher range as far as I remember. I just checked with g++ and it doesn't warn on implicit int->float on the highest warning level. I'd expect the divide itself to be enough documentation that the final value is a float.
But if I was going to cast, I'd explict cast via constructor form myself.
Anyway I'm just quibbling. Taking out the cast would have made the error more noticeable, as would have using the C++ cast form, although that's like 7 times as much typing for nothing. There's nothing wrong with the explicit cast being there, it's just redundant no matter which form you use.
Author
My point was just about the "evilness" of the "comma" operator [smile]
There are indeed a couple of better ways to write that code better.
There are indeed a couple of better ways to write that code better.
Quote:Original post by exwonderIt's probably compiler dependant, and without even bothering to check what mine does, I'll say anyway that numbers between 2^24 and 2^32 cannot be represented exactly in a 32-bit float, and thus there could be grounds for displaying a warning at the highest warning level of some compiler. Assuming of course that it can't prove that the intermediate calculation integer values in question are well below that in this case.
Compilers don't tend to warn about implicit casts to types with a higher range as far as I remember. I just checked with g++ and it doesn't warn on implicit int->float on the highest warning level. I'd expect the divide itself to be enough documentation that the final value is a float.
But if I was going to cast, I'd explict cast via constructor form myself.
Anyway I'm just quibbling. Taking out the cast would have made the error more noticeable, as would have using the C++ cast form, although that's like 7 times as much typing for nothing. There's nothing wrong with the explicit cast being there, it's just redundant no matter which form you use.
Obviously if the original code had 255.0 instead of 255.f then it couldn't be an issue.
But yes I too would generally be happy only explicitly making one of the operands a float, for the sake of less typing. Julian is right about what people should do though.
Quote:Compilers don't tend to warn about implicit casts to types with a higher range as far as I remember. I just checked with g++ and it doesn't warn on implicit int->float on the highest warning level. I'd expect the divide itself to be enough documentation that the final value is a float.
Hmmm, ok, VS05 warns on it, int->float can result in a loss of accuracy is the problem, if the integer is > 2^24 then converting to a float will result in a loss of accuracy, of coarse that cant actually happen in this case because all the values are guaranteed to be in the range 0 - 255 (perhaps gcc can detect this which is why it doesn't warn?). Any way at this point I guess it just comes down to personal preference.
Quote:My point was just about the "evilness" of the "comma" operator
There are indeed a couple of better ways to write that code better.
Most people will probably disagree with me here, however, I don't think that operator, is as evil as most people make out.
Exhibit A
void foo(std::vector<int>& lhs, std::vector<int>& rhs){ using namespace boost::lambda; std::transform( lhs.begin() , lhs.end() , rhs.begin() , rhs.begin() , ( std::cout << _1 , _1 = _ 2 * _2 , std::cout << _1 , _1 * _2 ) );}Exhibit B
void foo(std::vector<int>& values){ using namespace boost::assign; values += 1, 2, 3, 4, 5, 6, 7, 8, 9, 10;}Its extremely difficult to provide the facilities of boost::lambda without the use of operator, yet these facilities are extremely useful (although proper lambda expressions (coming in c++0x) would be more useful). In addition the ease of use provided by boost::assign and intuitive syntax can aid productivity and is far more intuitive then, for example, operator(), and less intrusive allowing it to work for non-standard library containers without modifying existing code.
Edit: To generalize I guess what I'm getting at is that operator, has its place even if it can occasionally cause problems its use embedding DSL's in c++ can result in greatly increased productivity and hence it isn't entirely evil.
(also fixed typo.)
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
