Fix Codable vtable layout #16057
Fix Codable vtable layout #16057
Conversation
|
@lorentey This introduces a mechanism for derived conformances to define class methods with deterministic vtable order. Just remember to update |
slavapestov
force-pushed
the
codable-vtable-layout
branch
2 times, most recently
from
e19a237 to
fa11300
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
| return elts.begin(); | ||
| } | ||
|
|
||
| decltype(elts)::const_iterator end() const { |
There was a problem hiding this comment.
I wonder if some of the less-capable C++ compilers we build with will handle this decltype idiom. I guess we'll find out
include/swift/SIL/SILVTableVisitor.h
Outdated
|
|
||
| synthesizedMembers.sort(); | ||
|
|
||
| for (auto pair : synthesizedMembers) { |
There was a problem hiding this comment.
const auto &pair to avoid the extraneous copies, please
lib/Sema/TypeCheckDecl.cpp
Outdated
| TC.requestSuperclassLayout(CD); | ||
|
|
||
| auto useConformance = [&](ProtocolDecl *protocol) { | ||
| (void)TC.conformsToProtocol( |
There was a problem hiding this comment.
This is going to check the conformance even if it's written on an extension, which will perform some unnecessary work. I guess that's going to be fairly rare for a class, but if you're concerned about it, you could drop the Used flag here and only force the completion of the conformance when it's DeclContext is the nominal type itself.
There was a problem hiding this comment.
Good point, fixed
lib/Sema/TypeCheckDecl.cpp
Outdated
| auto *decodableProto = Context.getProtocol(KnownProtocolKind::Decodable); | ||
| auto *encodableProto = Context.getProtocol(KnownProtocolKind::Encodable); | ||
| if (!evaluateTargetConformanceTo(decodableProto)) | ||
| (void)evaluateTargetConformanceTo(encodableProto); |
There was a problem hiding this comment.
You don't want to check for the name CodingKeys at all?
There was a problem hiding this comment.
It's checked with an early return at the top of the method
|
@slavapestov Thank you, this looks great! 🎉 |
|
This looks good too! A lot of the changes here were also made in #16054 too; should this subsume that PR? (Also, both PRs appear to have the same test failures in |
This ensures that the vtable of a class conforming to Encodable or Decodable always contains encode(to:) and init(from:), respectively. They might still get added in arbitrary order, however. This will be addressed in a subsequent patch. Mostly fixes <rdar://problem/35647420> and <https://bugs.swift.org/browse/SR-6468>.
Completes the fix for <rdar://problem/35647420> and <https://bugs.swift.org/browse/SR-6468>.
slavapestov
force-pushed
the
codable-vtable-layout
branch
from
fa11300 to
1602580
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
Fixes rdar://problem/35647420 and https://bugs.swift.org/browse/SR-6468.