[RESOLVED] cannot convert parameter 1 from 'ushort' to 'ushort &'

[Mybowlcut]

Guys there's something here that I'm not seeing... very tired so it's probably why.. but yeah:

The function I'm calling to:

void Inventory::SetCurrentItem(ushort& item_ID, const ushort& item_type)

The line I use to call the function:

SetCurrentItem(item.GetItemID(), item.GetType());

A ushort is an unsigned short int.
GetItemID() returns a ushort.
GetType() returns a ushort.

The error that I get:

Error	1	error C2664: 'Inventory::SetCurrentItem' : cannot convert parameter 1 from 'ushort' to 'ushort &'	c:\documents and settings\user\my documents\visual studio 2005\projects\hex\hex\inventory.cpp	779

[TheCPUWizard]

You need a reference to the ushort so the SetCurrentItem method can change the values. If the SetCurrentItem is not going to change the values, then whoever defined the interface did a poor job. Additionally "const ushort&" makes absolutely no sense [it is syntactically valid] as it does NOTHING but incur additional overhead. I seriously question the compentence of whoever designed the Inventory class....

[Zachm]

& before parameter passed to a function means that this parameter is passed by reference and if it is changed inside the function, the original variable would also be changed.

Them methods you use don't return a reference to the member, but the value of the members. This value is not a variable and cannot be referenced.

Do you need to change the member values of class item inside this function ?
If not - don't use & before the parameters to the function.
If you do need to change them inside the function, then this is bad design -
The same way you have implemented the Get..() methods, you should implement Set...() methods for your members you want to give external access to. To the function that needs to change the members you pass a class by reference like so:

 
SetCurrentItem(Item &item);

And inside the function you use the item.GetItemID() / item.GetType() as needed.

[MikeAThon]

If the above advice is not solving your problem, then I think that the problem is that you are trying to pass a temporary object into the SetCurrentItem function.

Temporary objects (i.e., the object created by item.GetItemID()) don't exeist beyond the function's call, so the compiler is telling you that it's treating the temporary object as a const. Once it does so, it doesn't match the signature of SetCurrentItem, which requires a non-const reference (presumably so that the ID can be changed).

To fix it, use a non-temporary object:

item_ID id = item.GetItemID();
SetCurrentItem(id, item.GetType());

Note that the second parameter can be a temportary one, since the function's signature calls for a const.

Mike

[TheCPUWizard]

MikeAThon, et. al.

That is being said about objects and tempoiraries is all correct. But look closely, the parameters are both unsigned (int's).

Now lets think about what a "const &unsigned" really does. It needs to take the ADDRESS of the variable (hence can not use a value), pass that address, and then reference (read only) the information pointed to by that address.

But the side of an unsigned is less than or equal to the size of a pointer, so no space is being saved, and the parameter is marked as a const so you can not perform modifications to it. This should definately be a "const unsigned" [value not reference], no memory menality, performance improvement, no security degradation.

Now the first parameter is harder to tell since it is not const. But just looking at the names [and being potentially very wrond], I have the feeling that the ID parameter is not being passed back out (ie expected to be modified in the calling context). If I am correct (again potentially wrong, we dont know the body of the method), then having this as a reference is also inappropriate.

[MikeAThon]

We might be talking about the same thing.

I should have mentioned the concept of l-value and r-value. In C++ expressions can evaluate to an l-value or an r-value. One not completely accurate way to think about these is that l-values can appear on the left side of an assignment statement whereas r-values cannot. For example:

int r = (3+3);  // valid: "r" is an lvalue and (3+3) is an rvalue
(3+3) = 7;  // not valid, because (3+3) is an rvalue

In the OP's code, the temporary ID returned by item.GetItemID() is an rvalue. It would not, for example, be permissible to write

item.GetItemID() = 7;  // not valid, item.GetItemID() is not an lvalue

A const reference can bind to an rvalue, whereas a non-const reference can only bind to an lvalue. Thus, if a function parameter is a non-const reference, the argument *must* be an lvalue, where if the function parameter is a const reference, then it is permissible for the argument to be either an rvalue or an lvalue.

If you think about it, the OP's code is the moral equivalent of the above, clearly incorrect code. Given the signature of SetCurrentItem, the first parameter is a non-const reference, so we expect that SetCurrentItem will change its value. So, is there any real difference in the following:

// signature of SetCurrentItem is
// void Inventory::SetCurrentItem(ushort& item_ID, const ushort& item_type)
// so, somewhere inside of SetCurrentItem, there might be line like
// item_ID = 7;

item.GetItemID() = 7;  // clearly incorrect, since item.GetItemID() is an rvalue
SetCurrentItem(item.GetItemID(), item.GetType());  // also incorrect, for the same reason

Mike

[TheCPUWizard]

Mike,

Good explaination

But I stand by my point that "const & unsigned" is basically a ______ signature.

[Mybowlcut]

-.-

I don't really see what's so bad about it... I made it and I was trying to increase performance by passing parts of an instance instead of the whole thing. The idea behind SetCurrentItem is that it creates a pointer to an item. Every item has an item_ID which IS used and has a VERY good reason to be used. item_type represents the type of item it is - weapon, armour, ring, etc. Those two variables were going to be passed in and then a pointer was going to be made to an item depending on what item_type was.

void Inventory::SetCurrentItem(ushort& item_ID, const ushort& item_type)
{
	--item_ID; //so can access all_items safely

	switch(item_type)
	{
		case 1: //weapon
		{
			current_weapon = &(all_items[item_ID]);
			break;
		}
		case 2: //armour
		{
			current_armour = &(all_items[item_ID]);
			break;
		}
		case 3: //gloves
		{
			current_gloves = &(all_items[item_ID]);
			break;
		}
		case 4: //boots
		{
			current_boots = &(all_items[item_ID]);
			break;
		}
		case 5: //amulets
		{
			current_amulet = &(all_items[item_ID]);
			break;
		}
		case 6: //rings
		{
			current_ring = &(all_items[item_ID]);
			break;
		}
		case 7: //arrows
		{
			current_gloves = &(all_items[item_ID]);
			break;
		}
		default:
		{
			throw runtime_error("Item type in IfBadThenMakeNull could not be determined -- please report this bug.");
			break;
		}
	}
}

The current_ pointers are used basically to have access to the current item of that type (weapon, etc).

EDIT: took the references out.. it works now and I understand why. Thanks.

[Paul McKenzie]

-.-

I don't really see what's so bad about it... I made it and I was trying to increase performance by passing parts of an instance instead of the whole thing.

First, your code is not really scalable. That switch() statement is going to get bigger and bigger if you decided to add more items. And I bet there are other switch() statements that look similar to this, where you are checking for the item type, am I right? It also uses a hard-coded "1", "2", etc. Those numbers by themselves don't convey what you're trying to do. Why not use an enum, so that it is much more understandable as to what the item_types are.

You do see that there is a pattern to that entire switch statement, where you could just index an array, right? Why not also pass the item you're setting? Then the switch can be eliminated altogether

void SetCurrentItem(ushort& item_ID,  ushort& item_type,
                                 whatever* item_to_set)
{
     --item_ID; //so can access all_items safely
      item_to_set = &all_items[item_ID];
}

But more to the point that others have brought up, and what you just mentioned in your last post:

These "micro-optimizations" that you think you are doing never amount to anything by the time the program has been compiled. You might as well do as ZachM suggested, and pass the Item to set.

Passing a reference to a basic type such as int, char, short, etc. should only be done if in the function, the parameter that is passed is being changed in the function. Otherwise it is a waste of time thinking you're speeding up anything.

By the time your code is optimized by the compiler, all of that code you believed you wrote to increase performance has been mangled, changed, and altered to something that you didn't write initially. If for some reason, passing a reference to a basic type would increase performance so dramatically, a smart optimizing compiler would change the code so that it would pass *all* of your parameters by reference.

What does increase performance is changing something that can be measured algorithmically. For example, if you can cut the number of times you loop down from 1,000,000 to 1,000, then that is a performance boost. Making sure that construction of an expensive object over and over again is another performance boost. Preferring a more efficient data structure to store your data, a different sorting algorithm, etc. are also more things that give you performance boosts.

Changing an if() to a switch(), passing a basic type by reference, and rinky-dink (sorry for the non-programming lingo) things like that do not get you any performance "boost". In some cases, it can get you a performance degradation, since you are trying to be cute with the code, and the optimizer for the compiler does not recognize the "cute" coding to optimize it properly. The classic case of this is when a programmer resorts to writing code using pointers all over the place, and the compiler cannot optimize the code properly due to pointer-aliasing issues.

And last, unless you profile your code, trying to code something that supposedly boosts performance, and to do this, your code becomes incomprehensible or hard to maintain, is just getting yourself into trouble. "Premature optimization is the root of all evil" is what Donald Knuth stated (or something similar to that). This means that you write the code first so that it makes sense and is understandable, and then optimize any code that is demonstrably slow.

Regards,

Paul McKenzie

[Mybowlcut]

Well, my project is huge... I wouldn't have a bloody clue where to start.. or share the slightest ideas on what is "good programming"... The reason this annoys me so $^#$^ much is because the code is really hard to change. Yeah, I know that's a bad thing. Basically, there's so much stuff in my program that's probably wrong after reading these replies, that it seems like a good idea to just ditch my code... which is... just gay pretty much. Bloody hell... I thought switch statements were good! Ack....