Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tests for NSHashTable/NSMapTable weak objects support #80

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

triplef
Copy link
Member

@triplef triplef commented Nov 4, 2019

I wrote these tests in order to track down the issues we’re seeing with weak pointer support in NSHashTable and NSMapTable that I outlined on the mailing list:
https://lists.gnu.org/archive/html/discuss-gnustep/2019-10/msg00008.html

The tests pass when run against Apple’s Foundation, but are failing for me when run on Android. I’d like to understand whether other configurations are affected as well (e.g. the CI test builds).

If they are failing on the CI I’d appreciate any fixes or insights into why that is the case. If they pass, please feel free to merge – we will then know to look for the issue in our toolchain configuration.

@triplef triplef requested a review from rfm as a code owner November 4, 2019 15:11
@triplef
Copy link
Member Author

triplef commented Nov 4, 2019

Looks like these issues are not specific to our setup. Any thoughts on why these tests might be failing? Unfortunately I don’t have a way to fully debug the issues on Android...

@triplef
Copy link
Member Author

triplef commented Nov 25, 2019

Looks like #84 will be fixing these tests. I will rebase once it’s merged.

@ngrewe
Copy link
Member

ngrewe commented Dec 7, 2019

Ah, this branch still fails with the gcc runtime, where NSPointerFunctionsWeakMemory behaves the same as NSPointerFunctionsZeroingWeakMemory, so the failure is rather unsurprising (we don't have GC enabled, so we end up with non-retained objects in the hash table). I think we should SKIP these tests – and maybe don't even define NSPointerFunctionsWeakMemory unless we are building base with an ARC-enabled runtime.

@triplef
Copy link
Member Author

triplef commented Dec 10, 2019

Good catch, thanks. I tried using OBJC_CAP_ARC to skip the tests with GCC, but it looks like the define is also not available with Clang/NG (even though it’s already being used in the NSProxy/basic test). Is there another define I could use here?

@ngrewe
Copy link
Member

ngrewe commented Dec 10, 2019

I tried using OBJC_CAP_ARC to skip the tests

You'll need to include <objc/capabilities.h> in order to get that macro. It's also just a constant describing the runtime capability and doesn't tell you whether the runtime was compiled with support for that capability (though in case of ARC, that should always be the case). To know whether the runtime actually supports it, you need to use it in conjunction with objc_test_capability() (the NSProxy/basic.m test gets that wrong).

@triplef
Copy link
Member Author

triplef commented Dec 10, 2019

Thanks @ngrewe! I’ve updated the tests to include capabilities.h and use objc_test_capability(), and the tests are now correctly skipped with GCC and pass with Clang. ✅

Looking at the NSProxy/basic test I think in that case the check might be correct, as it’s mirroring the same kind of checks in NSObject.m. It was all introduced as part of commit e35eb61.

@ngrewe
Copy link
Member

ngrewe commented Dec 10, 2019

Looking at the NSProxy/basic test I think in that case the check might be correct, as it’s mirroring the same kind of checks in NSObject.m

Yeah, that's probably functionally okay, since you can't compile libobjc2 without ARC support, it might just set a bit of a bad precedent for people looking at how to use the capabilities interface in libobjc2.

@fredkiefer fredkiefer merged commit 834e915 into gnustep:master Dec 10, 2019
@triplef triplef deleted the add-weak-table-tests branch June 3, 2020 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants