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

Update CMake supported clang versions #980

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

JJTech0130
Copy link
Contributor

@JJTech0130 JJTech0130 commented Jun 24, 2021

I also updated the docs to reflect this change: darlinghq/darling-docs#25

@JJTech0130 JJTech0130 mentioned this pull request Jun 24, 2021
@bugaevc
Copy link
Member

bugaevc commented Jun 24, 2021

To be honest, I'd like to keep old versions of Clang working too. If something we wrote doesn't work with them, I'd consider it a bug. However, if it's something Apple wrote (like objc4), then there's not much we could do.

@JJTech0130
Copy link
Contributor Author

JJTech0130 commented Jun 24, 2021

I mentioned some issues in darlinghq/darling-docs#25, and they were closed with "your clang is too old".
EDIT: Updated description with links to mentioned issues.

@bugaevc
Copy link
Member

bugaevc commented Jun 24, 2021

So objc_nonlazy_class looks like it can be trivially made to work on older Clang versions:

#if /* have new enough Clang */
__attribute__((objc_nonlazy_class))
#endif
@implementation Foo

#if /* have old Clang */
+ (void) load { }
#endif

(Empty load methods for NSObject and others is also what the older versions of objc4 use.)

cc @facekapow

@facekapow
Copy link
Member

Yeah, that look about right (not sure if that's all that older clangs would need, though). That's actually what OS_OBJECT_NONLAZY_CLASS and OS_OBJECT_NONLAZY_CLASS_LOAD in libdispatch do. The objc runtime would be the only one that needs to be updated (everything else uses libdispatch's macros).

@JJTech0130
Copy link
Contributor Author

JJTech0130 commented Jun 24, 2021

Well, um, I think that's ok. We should still add the new versions. Can I remove commits from the pull request, or is that not possible?

@facekapow
Copy link
Member

Yes; you can reset your branch back to master, and then make new commits, and then force push to the branch.

@JJTech0130
Copy link
Contributor Author

JJTech0130 commented Jun 24, 2021

Well, can't do that on mobile, and my pc is a bit stressed out trying to build darling... Will do it as soon as I finish.

@facekapow
Copy link
Member

Currently testing that potential fix for older clangs. I'm on Arch, though, so I can't test Clang 6 specifically (well, I could, but the AUR only has Clang 5 and then 7), only Clang 7.

@JJTech0130
Copy link
Contributor Author

OK. I have fixed the commits.

@JJTech0130
Copy link
Contributor Author

How should the docs be updated? The Debian 10 requirements specified clang-6.0, whilst there is a notice stating only clang 9+ is supported. Should the notice be changed? Removed? And should we leave the Debian 10 instructions or update them?

@facekapow
Copy link
Member

The docs should be updated to indicate that the minimum required Clang version is actually 6 now, not 9.

@JJTech0130
Copy link
Contributor Author

JJTech0130 commented Jun 25, 2021

darlinghq/darling-docs#26
I closed the other pull req. as there is no need to update the clang dep. for Debian 10 anymore.

@facekapow facekapow merged commit 26f4be2 into darlinghq:master Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TARGET_OS_DRIVERKIT missing under Clang 12
3 participants