open-ead / sead

Decompilation of sead: the standard C++ library for first-party Nintendo games
190 stars 26 forks source link

prim/ScopedLock: Fix makeScopedLock for pre-C++17 builds #106

Closed leoetlino closed 2 years ago

leoetlino commented 2 years ago

See https://github.com/open-ead/sead/pull/105#pullrequestreview-1064473605

@MonsterDruide1 let me know if this fixes makeScopedLock on Clang 3.9.


This change is Reviewable

MonsterDruide1 commented 2 years ago

No, still has the same issue as before:

/home/monsterdruide1/OdysseyDecompSead/OdysseyDecomp/lib/sead/modules/src/heap/seadHeapMgr.cpp:237:10: error: call to deleted constructor of 'sead::ScopedLock<sead::CriticalSection>'
    auto lock = makeScopedLock(sHeapTreeLockCS);
         ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/monsterdruide1/OdysseyDecompSead/OdysseyDecomp/lib/sead/include/prim/seadScopedLock.h:23:5: note: 'ScopedLock' has been explicitly marked deleted here
    ScopedLock(const ScopedLock& other) = delete;
    ^
leoetlino commented 2 years ago

Looks like this PR has fixed makeScopedLock at least, but not the usages.

You might need to do auto lock{makeScopedLock(...)}

MonsterDruide1 commented 2 years ago

Nope, still not working:

/home/monsterdruide1/OdysseyDecompSead/OdysseyDecomp/lib/sead/modules/src/heap/seadHeapMgr.cpp:237:10: error: call to deleted constructor of 'sead::ScopedLock<sead::CriticalSection>'
    auto lock{makeScopedLock(sHeapTreeLockCS)};
         ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/monsterdruide1/OdysseyDecompSead/OdysseyDecomp/lib/sead/include/prim/seadScopedLock.h:23:5: note: 'ScopedLock' has been explicitly marked deleted here
    ScopedLock(const ScopedLock& other) = delete;
    ^
leoetlino commented 2 years ago

Ugh I guess you still need C++17 copy/move elision for this to work. I think we can make this work by adding a move constructor to ScopedLock...

leoetlino commented 2 years ago

@MonsterDruide1 can you try again?

MonsterDruide1 commented 2 years ago

Now getting a different error:

/home/monsterdruide1/OdysseyDecompSead/OdysseyDecomp/lib/sead/include/prim/seadScopedLock.h:16:53: error: overload resolution selected deleted operator '='
    ScopedLock(ScopedLock&& other) noexcept { *this = other; }
                                              ~~~~~ ^ ~~~~~
leoetlino commented 2 years ago

Whoops, looks like I forgot to std::move the value... I'll fix this tonight

leoetlino commented 2 years ago

@MonsterDruide1 should be fixed now

MonsterDruide1 commented 2 years ago

Sorry for the delay, but - yes, this works correctly now, and results in the same match!