Fix swap() double-call

[NQNStudios]

After finding that #324 was due to swap() getting called twice, I figured out why the double-call was happening. For some reason, making the argument to the cUniverse copy constructor a reference and not a double-reference averts the bug. The double-reference looked wrong to me anyway, so maybe this is a correct solution.

[NQNStudios]

My fix might break other places where the universe changes, because now although parties have their items when they load, entering VoDT or On a Ship to Algiers and taking a single step causes this error:

Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: 20480
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: 20992
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: 21248
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: -256
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Unrecognized node type found: 23552
        Note: This could indicate corruption in the scenario, but more likely is just a result of garbage data in unused nodes or in the memory structures they were read into. The unrecognized node type has been replaced with invalid type -1.Loading scenario graphics... ("/home/nat/.config/openboe/blades/Scenarios/On a Ship to Algiers/Algiers.exs")
./test.sh: line 1: 342298 Segmentation fault      (core dumped) "./Blades of Exile"

[NQNStudios]

Ah, my fix isn't the cause of that problem. It can be reproduced on master.

[CelticMinstrel]

So, it's great that we found something that works, but I am still a little dubious about this change. The constructor you changed is supposed to be a move constructor, which takes an r-value reference (double &&). So while it's nice that this works, I think there must be something else wrong.

[NQNStudios]

That makes sense. I think what the debugger revealed is that the problematic line of code is calling both the move constructor AND std::move, which seems wrong. Changing the constructor signature must have caused it not to be called.

So I wonder if it would be more proper to make the load function’s universe argument into a pointer and assign the pointer’s value to std:: move instead. Maybe that will avoid calling the move constructor. That was the first approach I took but it introduced other compiler errors so I put a pin in it.

On Fri, Jan 27, 2023 at 6:28 PM Celtic Minstrel @.***> wrote:

So, it's great that we found something that works, but I am still a little dubious about this change. The constructor you changed is supposed to be a move constructor, which takes an r-value reference (double &&). So while it's nice that this works, I think there must be something else wrong.

— Reply to this email directly, view it on GitHub https://github.com/calref/cboe/pull/328#issuecomment-1407243698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATXBKM55NR5XQI7XNOIHCDWURY3FANCNFSM6AAAAAAUJIOJYA . You are receiving this because you authored the thread.Message ID: @.***>

[CelticMinstrel]

I think what the debugger revealed is that the problematic line of code is calling both the move constructor AND std::move, which seems wrong.

????

std::move does not do anything. It just returns its argument unchanged. Its only purpose is to convert an l-value reference to an r-value reference, which will then trigger the compiler to invoke the move constructor instead of the copy constructor.

By changing the signature of the move constructor, it no longer qualifies as a move constructor, so the compiler is probably calling the copy constructor instead.

Maybe that will avoid calling the move constructor.

I don't think avoiding the move constructor should be the goal here. Perhaps it's not correct for the move constructor to call swap in the first place. I'm not really sure though… it sounds correct to me…

Note that when I replaced real_univ = std::move(univ) with swap(real_univ, univ), I still got the bug.

[NQNStudios]

Ok disregard what I said about std::move, I thought it actually moved the data by calling swap. I don’t understand this stuff very well.

Swap() is definitely getting called twice, though…

On Sat, Jan 28, 2023 at 10:50 AM Celtic Minstrel @.***> wrote:

I think what the debugger revealed is that the problematic line of code is calling both the move constructor AND std::move, which seems wrong.

????

std::move does not do anything. It just returns its argument unchanged. Its only purpose is to convert an l-value reference to an r-value reference, which will then trigger the compiler to invoke the move constructor instead of the copy constructor.

By changing the signature of the move constructor, it no longer qualifies as a move constructor, so the compiler is probably calling the copy constructor instead.

Maybe that will avoid calling the move constructor.

I don't think avoiding the move constructor should be the goal here. Perhaps it's not correct for the move constructor to call swap in the first place. I'm not really sure though… it sounds correct to me…

Note that when I replaced real_univ = std::move(univ) with swap(real_univ, univ), I still got the bug.

— Reply to this email directly, view it on GitHub https://github.com/calref/cboe/pull/328#issuecomment-1407450239, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATXBKOVQWYZAJAKFXQ45TLWUVL7NANCNFSM6AAAAAAUJIOJYA . You are receiving this because you authored the thread.Message ID: @.***>

[NQNStudios]

I will share the actual callstacks of the two swap calls when I get a chance

On Sat, Jan 28, 2023 at 11:41 AM Nat Quayle Nelson < @.***> wrote:

Ok disregard what I said about std::move, I thought it actually moved the data by calling swap. I don’t understand this stuff very well.

Swap() is definitely getting called twice, though…

On Sat, Jan 28, 2023 at 10:50 AM Celtic Minstrel @.***> wrote:

I think what the debugger revealed is that the problematic line of code is calling both the move constructor AND std::move, which seems wrong.

????

std::move does not do anything. It just returns its argument unchanged. Its only purpose is to convert an l-value reference to an r-value reference, which will then trigger the compiler to invoke the move constructor instead of the copy constructor.

By changing the signature of the move constructor, it no longer qualifies as a move constructor, so the compiler is probably calling the copy constructor instead.

Maybe that will avoid calling the move constructor.

I don't think avoiding the move constructor should be the goal here. Perhaps it's not correct for the move constructor to call swap in the first place. I'm not really sure though… it sounds correct to me…

Note that when I replaced real_univ = std::move(univ) with swap(real_univ, univ), I still got the bug.

— Reply to this email directly, view it on GitHub https://github.com/calref/cboe/pull/328#issuecomment-1407450239, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATXBKOVQWYZAJAKFXQ45TLWUVL7NANCNFSM6AAAAAAUJIOJYA . You are receiving this because you authored the thread.Message ID: @.***>

[NQNStudios]

[NQNStudios]

party.cpp Line 167: swap(lhs.adven, rhs.adven); Lines 175-177:

for(size_t i = 0; i < lhs.adven.size(); i++) {
    swap(*lhs.adven[i], *rhs.adven[i]);
}

The adventurers are swapped back and forth in swap(cParty) !!

[NQNStudios]

oh, whoops, I force-pushed the other PR along with this one. Redoing

[CelticMinstrel]

Okay, so one is the move constructor and the other is the assignment operator… are they both swapping the same objects though?

[NQNStudios]

I think I understand why the assignment operator and move constructor are both being used here.

std::move() casts univ to cUniverse&& so it qualifies for the move constructor which returns cUniverse (as a temporary value).

Then the compiler has an assignment of cUniverse& = cUniverse. It resolves that to the operator= function.

swap() is therefore called twice and I think that might actually be correct?

The real problem was double-swapping the party.adven array, I think.

[CelticMinstrel]

Right, and the other thing is that the assignment operator takes its argument by value. So, in the move constructor, it should be swapping univ with a temporary, and then in the assignment operator it should be swapping that temporary with real_univ.

It is a bit silly that it swaps twice, but that doesn't appear to be the source of the issue. After all…

Note that when I replaced real_univ = std::move(univ) with swap(real_univ, univ), I still got the bug.