Skip to content
This repository was archived by the owner on Mar 28, 2020. It is now read-only.

Conversation

@hughbe
Copy link
Collaborator

@hughbe hughbe commented Jan 19, 2017

This seems to have been reset in stable/swift-4.0-branch after the update to LLVM 4.0

This is a port of #45 (the other stuff seems to still be there with MSVC).

This fixes an MSVC crash,

@jrose-apple, please let me know what other branch this should go to. Do I submit another duplicate PR for stable or upstream-with-swift?

@jrose-apple
Copy link
Contributor

The proper process is to put all LLVM/Clang changes into upstream-with-swift first and then cherry-pick them to the necessary release branches.

Rereading the original commit, I think probably the LLVM definition of None is wrong; it should be const NoneType None = NoneType::None;. Does that help anything or no?

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

Umm, that is the definition of None in LLVM:

https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/None.h#L24

@jrose-apple
Copy link
Contributor

jrose-apple commented Jan 19, 2017

Yes, I know. :-) I'm wondering if it's wrong, and if so if fixing it makes the error go away.

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

Haha, maybe I misunderstood your comment - you said "it should be ....`, but it already is!

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

I see now - I'm looking upstream rather than in swift-llvm! https://github.com/apple/swift-llvm/blob/swift-4.0-branch/include/llvm/ADT/None.h#L23

@jrose-apple
Copy link
Contributor

Yeah, back in #45 it used a funny self-referential thing. I'm not sure why ours is different.

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

I was under the impression everything was reset to upstream in the swift-4.0-branch. If this hasn't been updated, what else hasn't?

@jrose-apple
Copy link
Contributor

Either this just came in recently, or it's something we changed in upstream-with-swift. Let's see…looks like it is very recent. llvm-mirror/llvm@cf890be

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

Okay, should I cherry-pick that commit to upstream-with-swift as well as swift-4.0-branch

@jrose-apple
Copy link
Contributor

upstream-with-swift really is an "upstream"; it gets automerged frequently. This release with swift-4.0-branch is a little special in that we're also automerging from LLVM's release branch (coincidentally also version 4, I think). So if this does fix the problem, then ideally someone would cherry-pick it upstream.

(I think there'll be a Swift blog post about this soon, but there was at least a message on swift-dev.)

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

Awesome. All the upstream/stable/swift-x.y-branch stuff can get a little confusing :D. I assume we close this until upstream-with-swift, followed by stable, followed by swift-x.y-branch (not necessarily in that order) auto updates?

@jrose-apple
Copy link
Contributor

Last piece of the puzzle: "stable" is basically an alias for "the current release branch" to make cherry-picking easy. If we ever have a period where something goes into the next release but not this one, "stable" is where it would go, but so far that hasn't happened since the LLVM/Clang branches are prepared well before the Swift branches.

@hughbe
Copy link
Collaborator Author

hughbe commented Jan 19, 2017

Thanks for filling me in on this, appreciate it. Wonderfully complicated stuff...

@hughbe hughbe closed this Jan 19, 2017
@hughbe hughbe deleted the 4.0-msvc branch January 19, 2017 17:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants