Reference parameter returning copy

Question

For whatever reason, this reference parameter is returning a copy. So when I alter OutWeapon for index 0, it doesn't affect Weapon1. Am I doing this wrong?

class ULoadout
{
public:
    ULoadout();

    FWeaponSlot Weapon1;
    FWeaponSlot Weapon2;
    FWeaponSlot Weapon3;
    FWeaponSlot Weapon4;

    FSkillSlot Skill1;
    FSkillSlot Skill2;
    FSkillSlot Skill3;
    FSkillSlot Skill4;

    void GetWeapon(int32 InIndex, FWeaponSlot& OutWeapon);
    void GetSkill(int32 InIndex, FSkillSlot& OutSkill);
};

void ULoadout::GetWeapon(int32 InIndex, FWeaponSlot& OutWeapon)
{
    switch (InIndex)
    {
    case 0:
        OutWeapon = Weapon1;
        break;

    case 1:
        OutWeapon = Weapon2;
        break;

    case 2:
        OutWeapon = Weapon3;
        break;

    case 3:
        OutWeapon = Weapon4;
        break;

    default:
        break;
    }
}

Why are `Skill2` etc public? Why are you not using an array? — Ed Heal, Sep 27 '17 at 09:26
It looks like you are trying to assign a reference `Weapon1` to another reference `OutWeapon` and it doesn't do what you expect it to do. In C++ it is not possible to rebind references. assignment to reference always causes an assignment to the referenced object. — user7860670, Sep 27 '17 at 09:26
When you call `GetWeapon()`, you allocate memory for `OutWeapon` and then pass. What you really need is to return a pointer. — CinCout, Sep 27 '17 at 09:27
reference cannot be rebind. you might want pointer or `std::reference_wrapper`. — Jarod42, Sep 27 '17 at 09:28
or simply array: `FWeaponSlot weapons[4]; FWeaponSlot& GetWeapon(int32 i) { return weapons[i]; }` — Jarod42, Sep 27 '17 at 09:31
Perhaps you should actually ***return*** a reference instead? — Some programmer dude, Sep 27 '17 at 09:33
^That. Why do you return `void` and have an output parameter? [This](https://stackoverflow.com/questions/810797/which-is-better-return-value-or-out-parameter) is C# but the same applies — Passer By, Sep 27 '17 at 09:37
If all the members are public, you don't even *need* a getter. — Bo Persson, Sep 27 '17 at 09:50

score 4 · Accepted Answer · answered Sep 27 '17 at 09:38

4

Refactoring to

FWeaponSlot& ULoadout::GetWeapon(int32 InIndex)

is the sensible, and idiomatic, way of doing this. (C++ does not allow you rebind references.)

answered Sep 27 '17 at 09:38

Bathsheba

231,907
34
361
483

1

Should be mentioned, though, that the copying problem remains, if the result is not assigned to a reference again. If the target is a member of another class with independent lifetime (i. e. the member can exist before being assgned), it must be a pointer... – Aconcagua Sep 27 '17 at 09:45
@Sean: I'd be inclined to chuck out an exception in that case. – Bathsheba Sep 27 '17 at 10:21

score 0 · Answer 2 · answered Sep 27 '17 at 09:30

If I understand this correctly, a reference parameter can only be used to alter te object it references, but not to assign the refernce in the caller to a different object. For this, a pointer is necessary, which can be done as follows.

void ULoadout::GetWeapon(int32 InIndex, FWeaponSlot*& OutWeapon)
{
    switch (InIndex)
    {
    case 0:
        OutWeapon = &Weapon1;
        break;
    case 1:
        OutWeapon = &Weapon2;
        break;
    case 2:
        OutWeapon = &Weapon3;
        break;
    case 3:
        OutWeapon = &Weapon4;
        break;
    default:
        break;
    }
}

The caller would also need to give the address of the object to be exchanged. However, a different solution like returning a reference to the desired object or simply using an array index might be easier to understand.

score 0 · Answer 3 · answered Sep 27 '17 at 09:37

Consider how you call getWeapon function:

ULoadout u; // where ever you got it from...

FWeaponSlot s; // you need a true object!!!
u.getWeapon(0, s); // now you pass the true object in as reference!

What now happens in getWeapon effectively is:

s = weapon1;

Be aware that s is an object, not a reference, so you copy u.weapon1 into s...

To avoid this problem in the specific case you need a pointer instead:

FWeaponSlot* s = u.getWeapon(0);

Notice that I silently changed the signature (just for convenience on calling)...

u remains the owner of the objects and will clean them up automatically when being destroyed, so there is no need for any kind of smart pointers in this specific case. Just make sure that you do not use the s pointer after the lifetime of u ended...

score 0 · Answer 4 · answered Sep 27 '17 at 09:40

Whereas taking FWeaponSlot*& would solve your issue, I suggest to use regular getter instead:

class ULoadout
{
    FWeaponSlot weapons[4];
    FSkillSlot skills[4];
public:
    ULoadout();

    const FWeaponSlot& GetWeapon(int32 index) const { return weapons[index]; }
    FWeaponSlot& GetWeapon(int32 index) { return weapons[index]; }
    const FSkillSlot& GetSkill(int32 index) const { return skills[index]; }
    FSkillSlot& GetSkill(int32 index) { return skills[index]; }
};

score 0 · Answer 5 · answered Sep 27 '17 at 10:09

If you're set on using a reference then change the method to:

FWeaponSlot& ULoadout::GetWeapon(int32 InIndex);

However, you'll need to check that InIndex is valid within the method as you have no way of returning a value that indicates "no weapon".You'll want something like this at the top of the method:

if(InIndex > 3) throw std::out_of_range("invalid weapon index");

The other alternative is to return a pointer, and have null indicate "no weapon":

FWeaponSlot* ULoadout::GetWeapon(int32 InIndex);

Reference parameter returning copy

5 Answers5