-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Reflection] NFC: Enable building after clang r331013 #16212
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
[Reflection] NFC: Enable building after clang r331013 #16212
Conversation
@swift-ci please smoke test |
This appears to have broken the master-next bots: |
Hi @bob-wilson – Weird. This implies that the host compiler is top-of-tree-ish (at least enough to be considered "clang 7.0"). Is that correct? How do I find out what the last merge of clang into the host compiler is? Does it include r331013? |
I think I can fix this. What does |
I speculatively merged #16221. Please let me know if that doesn't fix it. |
Yes, the master-next branch builds with top-of-trunk Clang/LLVM. We build the runtime with the copy of Clang that gets built with Swift, so that will also be top-of-trunk. |
Can you confirm that |
No, APPLE_CC isn't the right thing. (That macro isn't being updated and should be removed but that's a lot of hassle and a low priority so no one has gotten to it.) The real problem here is that Swift is hard-coding a default of 5.0.0 for CLANG_USER_VISIBLE_VERSION in utils/build_swift/defaults.py. That value hasn't been updated in a while. |
So what's the right way to detect the underlying llvm/clang version then? |
I was just asking @jrose-apple about that. It sounds like any sort of version check is not going to work well. Jordan suggested a __has_feature check, but the change in r331013 looks to me like a bug fix. If that's really the case, it seems like there ought to be a way to write that code so that it works with both old and new compilers. |
Uh… I don't know what to do. Clang, to the best my understanding, went from requiring |
I'll try some things.... |
The Clang version checks introduced in swiftlang#16212 and swiftlang#16221 do not work well. The __clang_major__ macro is defined differently for Apple builds of Clang, and the __APPLE_CC__ macro is obsolete and not useful. The good news is that those checks appear to be unnecessary. The version of Clang used with Swift 4.2 (based on LLVM 6.0) accepts the code without the template keyword in the places where the DEPENDENT_TEMPLATE2 macro was being used. This also corrects a mistake in swiftlang#16221 where the non-Clang/GCC definition of DEPENDENT_TEMPLATE was changed to use the template keyword.
It doesn't look like Clang (at least the versions we care about) requires "template" in those cases. See #16372 |
The Clang version checks introduced in swiftlang#16212 and swiftlang#16221 do not work well. The __clang_major__ macro is defined differently for Apple builds of Clang, and the __APPLE_CC__ macro is obsolete and not useful. The good news is that those checks appear to be unnecessary. The version of Clang used with Swift 4.2 (based on LLVM 6.0) accepts the code without the template keyword in the places where the DEPENDENT_TEMPLATE2 macro was being used. This also corrects a mistake in swiftlang#16221 where the non-Clang/GCC definition of DEPENDENT_TEMPLATE was changed to use the template keyword.
The Clang version checks introduced in swiftlang#16212 and swiftlang#16221 do not work well. The __clang_major__ macro is defined differently for Apple builds of Clang, and the __APPLE_CC__ macro is obsolete and not useful. The good news is that those checks appear to be unnecessary. The version of Clang used with Swift 4.2 (based on LLVM 6.0) accepts the code without the template keyword in the places where the DEPENDENT_TEMPLATE2 macro was being used. This also corrects a mistake in swiftlang#16221 where the non-Clang/GCC definition of DEPENDENT_TEMPLATE was changed to use the template keyword.
No description provided.