-
Notifications
You must be signed in to change notification settings - Fork 10
Simplify containers to use only a single container protocol. #14
Simplify containers to use only a single container protocol. #14
Conversation
I don't think that I will make it work with Swift 5.1. It works with Swift 5.2. On my local, the Swift 5.1 toolchain (macOS) can't even compile the (valid!) source because of a Swift compiler bug, so I have a hard time finding a good approach to fix the errors that happen on CI:
I see a couple of options that we have now:
Let me know what you think, @mattt. Either is fine for me. |
@Lukas-Stuehrk I'm sorry that this isn't working. Though, I must admit to feeling a small bit of validation about my hacky implementation (I knew there was a reason for all of that copy-paste!) But I totally agree that this is a superior approach, and it should be the way things are done going forward. In terms of release planning, how about we schedule this and #13 for a 1.0.0 release targeting Swift 5.2? Those two PRs feel like a major milestone on their own, and making this a new major version mitigates any inconvenience for downstream adoption. Sound like a plan? |
@Lukas-Stuehrk Agreed on all counts. In the interest of avoiding a separate |
a56a651
to
fdc5ff4
Compare
Until we reach 1.0.0 we're good to do breaking changes if necessary. 0.x.y updates are exempt from the semver rules. So we can make the necessary changes one by one without stashing them on a develop branch, no? |
@regexident It is true that SemVer permits any changes to the public API before version 1.0.0, but that only contains how subsequent releases are numbered; branching strategies between releases is a separate concern. So long as |
|
} | ||
extension Container where ChildNode: Node { | ||
public init(children: [ChildNode]) { | ||
self.init(cmark_node_new(type(of: self).cmark_node_type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In context of #14 (comment) I'd guess this line to be the culprit.
Have you tried replacing it with this?
self.init(cmark_node_new(Self.cmark_node_type))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've tried already. It will indeed make this Swift compiler crash on Linux go away. Unfortunately, it then runs in yet another Swift compiler crash. All the convenience initializers on the nodes which take children cause the following compiler crash:
swift: /home/buildnode/jenkins/workspace/oss-swift-5.2-package-linux-ubuntu-18_04/swift/lib/IRGen/GenPoly.cpp:70: void swift::irgen::reemitAsUnsubstituted(swift::irgen::IRGenFunction &, swift::SILType, swift::SILType, swift::irgen::Explosion &, swift::irgen::Explosion &): Assertion `expectedSchema.size() == cast<LoadableTypeInfo>(substTI).getExplosionSize()' failed.
fdc5ff4
to
4234f84
Compare
I pushed my progress so far. The library itself is compiling, both on macOS and on Linux. There's one crash of the Swift compiler left on Linux when trying to build the tests. The culprits are all the insertions in the manipulation tests, e.g. https://github.com/SwiftDocOrg/CommonMark/blob/master/Tests/CommonMarkTests/ContainerManipulationTests.swift#L22 |
@mattt I tried every possible mitigation which I could think of. I will not make this work on Linux (yet). To unblock things: How about we drop this idea for now and unblock other pull requests? And then I'll have a look again once Swift 5.3 has landed. It still feels right to do it. |
@Lukas-Stuehrk I'm really sorry to hear that didn't pan out. Thank you so much for digging into this. I learned a lot from looking through your code and discussion. Let's put a pin in this for now and reassess later this year, when Swift 5.3 is available. |
Looking forward to giving this another go with 5.3! |
@regexident Indeed it does! I can take another look at everything soon. Anything left to do there? |
No, should be fine. |
Implements #11