[ClangImporter] Support Swift 5 API notes #11867
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Preparation for making ImportNameVersion a generalized struct rather than an enum. We could have kept cramming it into a bitfield, sure, but we don't actually need this. No intended functionality change.
jrose-apple
force-pushed
the
ClangImporter-episode-V
branch
from
cb7ce40 to
dcb12e8
Compare
milseman
approved these changes
Sep 12, 2017
lib/ClangImporter/ImporterImpl.h
Outdated
| @@ -1129,7 +1131,7 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation | |||
| void insertMembersAndAlternates(const clang::NamedDecl *nd, | |||
| SmallVectorImpl<Decl *> &members); | |||
| void loadAllMembersIntoExtension(Decl *D, uint64_t extra); | |||
| void addMemberAndAlternatesToExtension( | |||
| bool addMemberAndAlternatesToExtension( | |||
There was a problem hiding this comment.
What does the return value signify? Could you add a comment?
DougGregor
approved these changes
Sep 14, 2017
|
apple/swift-clang#126 |
|
Build failed |
@fredriss, I thought we fixed this? Did the fix not make it into swift-4.1-branch? |
|
@jrose-apple Swift is supposed to have it's own fix for this. The one we use on the current branch in clang won't have any effect on the swift build. |
|
Um. LLVM should just get this right. I'll take a look. |
|
We have a generic fix on master, but it wasn't cherry-picked to the current stable branch |
|
Hm. Any reason not to do so? Seems safe and correct. |
|
No reason except that we didn't seem to need it. |
|
apple/swift-clang#126 |
1 similar comment
|
apple/swift-clang#126 |
forEachDistinctName might produce the same name for Swift 4 and Swift 5, but it's possible that for some reason the name will only work in one mode or the other. In that case, even though we're trying the "same" name again, we still want to invoke the callback once more. Add a boolean return to the callback to support this. Tests to come at the end of this patch series -- this shows up when in Swift 3 mode and the canonical version for types is set to Swift 5.
...so that we don't have to keep coming back to update it every major release. And also so we can actually put methods on it instead of using free functions. No intended behavior change (yet).
*** Depends on Clang change "[APINotes] Record what version caused *** *** an annotation to get replaced." Update your Clang checkout! *** More generally, change the meaning of the SwiftVersions section in API notes to be "this version or earlier" rather than "exactly this version". We mostly get this behavior for free from the Clang-side changes, but for SwiftName and the enum annotations we look at inactive attributes as well. The latter is simple, but the former means being careful about finding the annotation we /would/ have picked, i.e. the one closest to the version we requested.
"...finally." This was technically enabled two commits ago, since nothing checks that you're not /over/ maxVersion(). This is only used for - deciding the canonical way to import renamed types - trying to import things in multiple ways ...and so there are very few observable differences, especially before anyone has added any API notes that differentiate Swift 4 and Swift 5. At some point we should start encoding name versions in the lookup tables so that we only have to try all the names /once/, but the test suite doesn't seem to get measureably slower with this change, probably because it's pretty quick to decide that most things don't have multiple names. So we can put that off until later.
jrose-apple
force-pushed
the
ClangImporter-episode-V
branch
from
dcb12e8 to
a0117b0
Compare
|
Passed full testing before fixing the comment. apple/swift-clang#126 |
|
apple/swift-clang#126 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor name importing and the ImportNameVersion type in particular to make it easier to add a new Swift version…then actually do so. This finishes off the work in #11507; we can now use -swift-version 5 end-to-end.
Depends on apple/swift-clang#122 and apple/swift-clang#123, or rather their inclusion into the Clang swift-4.1-branch. Tests pass locally, of course.