[WIP] Added -swift-version support for 4.2 #15407
Conversation
Further, when ‘4’ is specified on its own for the language version without a minor component, assume ‘4.1’ as the language version.
|
@jrose-apple This is incomplete. I just wanted to make sure this looks on the right track as far as specifying the version mode to "4.1" with --swift-version 4. |
lib/Basic/Version.cpp
Outdated
| @@ -326,6 +327,9 @@ Optional<Version> Version::getEffectiveLanguageVersion() const { | |||
| case 4: | |||
| static_assert(SWIFT_VERSION_MAJOR == 4, | |||
| "getCurrentLanguageVersion is no longer correct here"); | |||
| // Version '4' on its own implies '4.1'. | |||
| if (size() == 1) | |||
| return Version{4, 1}; | |||
| return Version::getCurrentLanguageVersion(); | |||
There was a problem hiding this comment.
I feel like this should at least check for 4.0 and 4.1.
There was a problem hiding this comment.
Check in what way? The driver will disallow passing "4.0" and "4.1" explicitly. The only thing the driver will allow is passing "4.2". We could add an assert that, if there is a second component, that it is "2". WDYT?
There was a problem hiding this comment.
Oh, right, the allowed-language-versions check. Okay, yes, that assert would be good enough for me.
There was a problem hiding this comment.
Ah, it shouldn't be 4.1, though. It needs to be higher than any 4.1 point updates we release, right? So 4.1.99 or something.
There was a problem hiding this comment.
I spoke wrong. The allowed-language-versions check is defined in terms of getEffectiveLanguageVersion.
…ase. Needed to support Swift 4.2.
|
@jrose-apple I added the necessary logic to teach the ClangImporter about 4.2 and to handle minor versions. It's not very pretty. Most of it is OK, except:
|
|
@swift-ci smoke test |
| // We encode the 'rawValue' as just major version numbers with the | ||
| // exception of '4.2', which is a special minor version that can impact | ||
| // importing of names. We treat that with a rawValue of 5, and treat | ||
| // all major values of 5 or higher as being rawValue = majorversion + 1. |
There was a problem hiding this comment.
Since we don't distinguish between Swift 1 and Swift 2, what do you think about moving the raw values down instead of up? It's a trick that will only work once, but even so.
| @@ -259,7 +259,7 @@ SwiftVersions: | |||
| SwiftName: multiVersionedGlobal45Notes_5 | |||
| - Name: multiVersionedGlobal45Both | |||
| SwiftName: multiVersionedGlobal45Both_5 | |||
| - Version: 4 # Versions are deliberately ordered as "3, 5, 4" to catch bugs. | |||
| - Version: 4 # Versions are deliberately ordered as "3, 5, 4.2, 4" to catch bugs. | |||
There was a problem hiding this comment.
Can you add a separate test that shows that "Version: 4.2" with no other versions specified also affects -swift-version 4 and -swift-version 3?, and that "Version: 4" with no other versions does not affect -swift-version 4.2? Or maybe that's in here already; the original 3-4-5 tests did do things like that.
There was a problem hiding this comment.
@jrose-apple I added this in the latest commit.
|
I do agree about the 4.2 being repeated. I'll see if I can think of a better way as well. |
This tests that a “Version 4.2” change impacts -swift-version 3 and 4, but not 5.
|
@swift-ci smoke test |
|
@jrose-apple what do you think about landing this as is, and further refinements to |
No description provided.