Skip to content

[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

Merged

Conversation

davezarzycki
Copy link
Contributor

No description provided.

@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test

@davezarzycki davezarzycki merged commit 4732be1 into swiftlang:master Apr 27, 2018
@davezarzycki davezarzycki deleted the nfc_build_after_clang_r331013 branch April 27, 2018 13:01
@bob-wilson
Copy link
Contributor

This appears to have broken the master-next bots:
https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/

@davezarzycki
Copy link
Contributor Author

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?

@davezarzycki
Copy link
Contributor Author

I think I can fix this. What does __APPLE_CC__ correspond to? The underlying clang version?

davezarzycki added a commit to davezarzycki/swift that referenced this pull request Apr 27, 2018
@davezarzycki
Copy link
Contributor Author

I speculatively merged #16221. Please let me know if that doesn't fix it.

@bob-wilson
Copy link
Contributor

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.

@davezarzycki
Copy link
Contributor Author

Can you confirm that __APPLE_CC__ equals the underlying clang/llvm version?

nathawes pushed a commit to nathawes/swift that referenced this pull request Apr 28, 2018
@bob-wilson
Copy link
Contributor

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.

@davezarzycki
Copy link
Contributor Author

So what's the right way to detect the underlying llvm/clang version then?

@bob-wilson
Copy link
Contributor

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.

@davezarzycki
Copy link
Contributor Author

Uh… I don't know what to do. Clang, to the best my understanding, went from requiring template in some contexts to failing if the keyword exists.

@bob-wilson
Copy link
Contributor

I'll try some things....

bob-wilson added a commit to bob-wilson/swift that referenced this pull request May 4, 2018
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.
@bob-wilson
Copy link
Contributor

It doesn't look like Clang (at least the versions we care about) requires "template" in those cases. See #16372

bob-wilson pushed a commit to bob-wilson/swift that referenced this pull request Sep 14, 2018
bob-wilson pushed a commit to bob-wilson/swift that referenced this pull request Sep 14, 2018
bob-wilson added a commit to bob-wilson/swift that referenced this pull request Sep 14, 2018
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.
mikeash pushed a commit to mikeash/swift that referenced this pull request Nov 30, 2018
mikeash pushed a commit to mikeash/swift that referenced this pull request Nov 30, 2018
mikeash pushed a commit to mikeash/swift that referenced this pull request Nov 30, 2018
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.
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.

2 participants