Sign in to follow this  

Try to find the bug :)

This topic is 3837 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hey! A little challenge for you guys [smile]
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 


Enjoy :)

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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
);
Fixed.

Share this post


Link to post
Share on other sites
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
);

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites
Quote:
Original post by exwonder

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.
It'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.
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.

Share this post


Link to post
Share on other sites
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.)

Share this post


Link to post
Share on other sites

This topic is 3837 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this