New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate mirrors to use CustomReflectable API, rewrite dump() #838

Merged
merged 1 commit into from Jan 22, 2016

Conversation

Projects
None yet
5 participants
@austinzheng
Contributor

austinzheng commented Jan 1, 2016

Jira: SR-88
Changes:

  • Removed stdlib type conformances to _Reflectable
  • Conformed stdlib types to CustomReflectable, CustomPlaygroundQuickLookable
  • Rewrote dump() function to not use _reflect()
  • Rewrote unit tests for compatibility with new API

NOTES: Jira here: https://bugs.swift.org/browse/SR-88. The commit turned out quite a bit larger than I anticipated, so if you want me to try breaking it down I'd be happy to do so. All tests pass on OS X. Note that I disabled a few tests that were affected by the linked bugs in the Jira ticket. A later PR will contain additional unit tests for any types that I missed. Let me know if you have any questions.

@austinzheng

View changes

Show outdated Hide outdated test/1_stdlib/Reflection_objc.swift Outdated
@austinzheng

View changes

Show outdated Hide outdated stdlib/public/core/Reflection.swift Outdated
@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 1, 2016

Member

Very awesome to see this finally getting done, Austin! Just one big question: I expected to see all the gyb’d nonsense fall away when we got to this point. Is it really still pulling its weight?

Member

dabrahams commented Jan 1, 2016

Very awesome to see this finally getting done, Austin! Just one big question: I expected to see all the gyb’d nonsense fall away when we got to this point. Is it really still pulling its weight?

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 1, 2016

Contributor

Thanks! I would say that gyb is still pulling its weight in files like Arrays.swift.gyb and CGFloat.swift.gyb, where mirror conformance was only a small part. However, there are a bunch of mirror-specific .gyb files that I could get rid of, moving the conformances directly into the 'base' source files for each type. That might improve code structure/readability at the cost of a little duplication. Let me know what your thoughts are, and I'll make the appropriate changes for the next revision of this PR.

Contributor

austinzheng commented Jan 1, 2016

Thanks! I would say that gyb is still pulling its weight in files like Arrays.swift.gyb and CGFloat.swift.gyb, where mirror conformance was only a small part. However, there are a bunch of mirror-specific .gyb files that I could get rid of, moving the conformances directly into the 'base' source files for each type. That might improve code structure/readability at the cost of a little duplication. Let me know what your thoughts are, and I'll make the appropriate changes for the next revision of this PR.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 1, 2016

Contributor

One further note - there's one remaining use of _reflect(), in OutputStream.swift's _adHocPrint() method. I didn't remove it because that method depends on switching on the concrete type of the _MirrorType instance returned by _reflect(), and then doing stuff based on it. I can hold off removing it if there's a plan to replace or rewrite that method, or I can rewrite it myself. Up to you.

Contributor

austinzheng commented Jan 1, 2016

One further note - there's one remaining use of _reflect(), in OutputStream.swift's _adHocPrint() method. I didn't remove it because that method depends on switching on the concrete type of the _MirrorType instance returned by _reflect(), and then doing stuff based on it. I can hold off removing it if there's a plan to replace or rewrite that method, or I can rewrite it myself. Up to you.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 2, 2016

Member

@austinzheng yes, all the mirror-specific .gyb files, and the mirror-specific gyb usage within files that are gyb’d for other reasons, were what I had in mind. What kind of code duplication do you envision? The current design of Mirror was specifically crafted to avoid that duplication and the need for gyb.

Member

dabrahams commented Jan 2, 2016

@austinzheng yes, all the mirror-specific .gyb files, and the mirror-specific gyb usage within files that are gyb’d for other reasons, were what I had in mind. What kind of code duplication do you envision? The current design of Mirror was specifically crafted to avoid that duplication and the need for gyb.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 2, 2016

Member

@austinzheng I don’t see why that switching should be needed since all the _MirrorType models would be disappearing?

Member

dabrahams commented Jan 2, 2016

@austinzheng I don’t see why that switching should be needed since all the _MirrorType models would be disappearing?

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 2, 2016

Contributor

@dabrahams Sounds good, I'll remove the mirror-specific gyb usage. "Code duplication" was more of a strawman than anything else; I don't think it's an actual issue.

As for _adHocPrint(), what I meant was that the existing implementation switches on _MirrorType's concrete type. I'll rewrite that function to not depend on _reflect(), if you have no objections.

Contributor

austinzheng commented Jan 2, 2016

@dabrahams Sounds good, I'll remove the mirror-specific gyb usage. "Code duplication" was more of a strawman than anything else; I don't think it's an actual issue.

As for _adHocPrint(), what I meant was that the existing implementation switches on _MirrorType's concrete type. I'll rewrite that function to not depend on _reflect(), if you have no objections.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 2, 2016

Contributor

One thing I need to figure out is how to tell that a Mirror is reflecting a metatype. The existing code checks if the _MirrorType is a _MetatypeMirror, which isn't a good solution going forward. The Mirror API doesn't provide access to the object being reflected, and the subjectType property (whose type is Any.Type) doesn't seem to provide enough information to disambiguate a metatype from a metatype type without resorting to something unacceptably hacky (like checking for a ".Type" suffix on the type name). I'll think about this some more.

Contributor

austinzheng commented Jan 2, 2016

One thing I need to figure out is how to tell that a Mirror is reflecting a metatype. The existing code checks if the _MirrorType is a _MetatypeMirror, which isn't a good solution going forward. The Mirror API doesn't provide access to the object being reflected, and the subjectType property (whose type is Any.Type) doesn't seem to provide enough information to disambiguate a metatype from a metatype type without resorting to something unacceptably hacky (like checking for a ".Type" suffix on the type name). I'll think about this some more.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 2, 2016

Member

Rewriting _adHocPrint sounds like the right move. We want to retire _MirrorType.

Member

dabrahams commented Jan 2, 2016

Rewriting _adHocPrint sounds like the right move. We want to retire _MirrorType.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 2, 2016

Member

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.

Member

dabrahams commented Jan 2, 2016

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 2, 2016

Contributor

You're absolutely right, the original object is in fact passed to the function. I'll be sure to get some fresh air before I write any more code :).

On Jan 1, 2016, at 6:14 PM, Dave Abrahams notifications@github.com wrote:

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.


Reply to this email directly or view it on GitHub #838 (comment).

Contributor

austinzheng commented Jan 2, 2016

You're absolutely right, the original object is in fact passed to the function. I'll be sure to get some fresh air before I write any more code :).

On Jan 1, 2016, at 6:14 PM, Dave Abrahams notifications@github.com wrote:

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.


Reply to this email directly or view it on GitHub #838 (comment).

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 2, 2016

Contributor

The last obstacle before I can finish rewriting my function is getting the string representing a particular enum case. (The generic mirror created by Mirror(reflecting:) doesn't have any case name information.)

The way it works right now is that _EnumMirror has a property called caseName, which in turn calls into a C++ runtime function named swift_EnumMirror_caseName. This property allows the _EnumMirror to get a reference to the name string from the runtime. I've tried calling into this function from outside the context of _EnumMirror, but with limited success. I'll draft an email to swift-dev later asking for help understanding the context of that runtime entry point.

Contributor

austinzheng commented Jan 2, 2016

The last obstacle before I can finish rewriting my function is getting the string representing a particular enum case. (The generic mirror created by Mirror(reflecting:) doesn't have any case name information.)

The way it works right now is that _EnumMirror has a property called caseName, which in turn calls into a C++ runtime function named swift_EnumMirror_caseName. This property allows the _EnumMirror to get a reference to the name string from the runtime. I've tried calling into this function from outside the context of _EnumMirror, but with limited success. I'll draft an email to swift-dev later asking for help understanding the context of that runtime entry point.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 2, 2016

Member

I think @jckarter is the knower of these things; hopefully this mention gets his attention.

Member

dabrahams commented Jan 2, 2016

I think @jckarter is the knower of these things; hopefully this mention gets his attention.

@@ -121,7 +120,6 @@ set(SWIFTLIB_SOURCES
### PLEASE KEEP THIS LIST IN ALPHABETICAL ORDER ###
Availability.swift
Bit.swift
CollectionMirrors.swift.gyb
CollectionOfOne.swift
ExistentialCollection.swift.gyb
Mirror.swift

This comment has been minimized.

@dabrahams

dabrahams Jan 6, 2016

Member

Speaking of Mirror.swift, the _legacyMirror method should obviously be gone when this PR is done.

@dabrahams

dabrahams Jan 6, 2016

Member

Speaking of Mirror.swift, the _legacyMirror method should obviously be gone when this PR is done.

This comment has been minimized.

@austinzheng

austinzheng Jan 19, 2016

Contributor

Hi Dave,

_legacyMirror is tightly integrated with the Mirror initializers that generate mirrors from _reflect() if no custom mirror is available. I don't think it can be removed until Phase 3 of SR-88 is complete, at which point we'd rip all use of _reflect() out of Mirror and use the new runtime entry points.

What do you think?

@austinzheng

austinzheng Jan 19, 2016

Contributor

Hi Dave,

_legacyMirror is tightly integrated with the Mirror initializers that generate mirrors from _reflect() if no custom mirror is available. I don't think it can be removed until Phase 3 of SR-88 is complete, at which point we'd rip all use of _reflect() out of Mirror and use the new runtime entry points.

What do you think?

This comment has been minimized.

@gribozavr

gribozavr Jan 19, 2016

Collaborator

I think it makes sense to make as much progress as possible in the incremental development spirit. If we can't remove something without redesigning the runtime, it shouldn't block progress where we can make it.

@gribozavr

gribozavr Jan 19, 2016

Collaborator

I think it makes sense to make as much progress as possible in the incremental development spirit. If we can't remove something without redesigning the runtime, it shouldn't block progress where we can make it.

This comment has been minimized.

@austinzheng

austinzheng Jan 19, 2016

Contributor

Agreed, this PR is already pretty big as-is. I'm happy to do follow-up work that only makes sense after other dependencies are resolved.

@austinzheng

austinzheng Jan 19, 2016

Contributor

Agreed, this PR is already pretty big as-is. I'm happy to do follow-up work that only makes sense after other dependencies are resolved.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 7, 2016

Contributor

I'm still working on this. Goals for next update:

  • Remove use of gyb for mirrors
  • Rewritten _adHocPrint(), including way to print enum case names
  • Remove _legacyMirror and friends
Contributor

austinzheng commented Jan 7, 2016

I'm still working on this. Goals for next update:

  • Remove use of gyb for mirrors
  • Rewritten _adHocPrint(), including way to print enum case names
  • Remove _legacyMirror and friends
@slavapestov

View changes

Show outdated Hide outdated stdlib/public/core/ImplicitlyUnwrappedOptional.swift Outdated
@slavapestov

View changes

Show outdated Hide outdated stdlib/public/core/Optional.swift Outdated
@slavapestov

View changes

Show outdated Hide outdated stdlib/public/core/Reflection.swift Outdated
@slavapestov

View changes

Show outdated Hide outdated test/1_stdlib/Runtime.swift Outdated
@@ -190,10 +190,10 @@ func unarchive() {
fatalError("unable to unarchive Foo<NSNumber>")
}
// CHECK-LABEL: Foo<NSString>
// CHECK-LABEL: <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0

This comment has been minimized.

@slavapestov

slavapestov Jan 10, 2016

Member

This looks like a QoI regression to me

@slavapestov

slavapestov Jan 10, 2016

Member

This looks like a QoI regression to me

This comment has been minimized.

@austinzheng

austinzheng Jan 10, 2016

Contributor

Agreed. I'll have to look into that specific case, but in general getting rid of summary has made some of the descriptions less legible. I posted a thread about it in swift-evolution last week, but it's currently inactive.

@austinzheng

austinzheng Jan 10, 2016

Contributor

Agreed. I'll have to look into that specific case, but in general getting rid of summary has made some of the descriptions less legible. I posted a thread about it in swift-evolution last week, but it's currently inactive.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 19, 2016

Contributor

I updated this PR. (As part of the update I did some rebasing and force pushing; it looks like the PR is still intact.)

Slava pointed out a few things that should be addressed before this goes in. I'll also need to spin up a Ubuntu VM and run the tests there.

Contributor

austinzheng commented Jan 19, 2016

I updated this PR. (As part of the update I did some rebasing and force pushing; it looks like the PR is still intact.)

Slava pointed out a few things that should be addressed before this goes in. I'll also need to spin up a Ubuntu VM and run the tests there.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 19, 2016

Contributor

I removed a few more GYB files. I left GYB mirror implementations when it 'made sense' - e.g. there was no good place to put the code otherwise, since a type's entire implementation was in a GYB file, or moving the mirrors out of the GYB would have caused significant repetition.

Contributor

austinzheng commented Jan 19, 2016

I removed a few more GYB files. I left GYB mirror implementations when it 'made sense' - e.g. there was no good place to put the code otherwise, since a type's entire implementation was in a GYB file, or moving the mirrors out of the GYB would have caused significant repetition.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 20, 2016

Contributor

I pushed an update.

CGRect, CGPoint, and CGSize now conform to CustomDebugStringConvertible.

dump() now has a new technique for printing a description of the item being dumped, which is designed to mirror the old behavior:

  1. If the mirror display style is one of the following aggregate kinds, the description is in terms of number of children. (This relies on the idea that a collection's reflected children are synonymous with the items it contains.)
    • Collections are described like so: 5 elements
    • Dictionarys are described like so 5 key/value pairs
    • Sets are described like so: 5 members
    • Tuples are described like so: (5 elements)
  2. If the object conforms to CustomStringConvertible, CustomDebugStringConvertible, or Streamable, the object is allowed to describe itself.
  3. If the object does not conform and has a display style of Class, Struct, or Enum, the description is the fully-qualified type name + the enum case name (if applicable).
  4. Otherwise, the description is generated using _adHocPrint().

print(), debugPrint(), and all other APIs are unaffected by these changes.

I figured the work to make the above possible was simple enough that it didn't need to be discussed extensively before an initial implementation. If anyone thinks that the algorithm above should be changed, I'm happy to update the PR.

Unfortunately, the regression that @slavapestov mentioned in a line comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a description or a debugDescription, and it defaults to something like the mangled type name + address.

Many of the test changes that were in previous versions of this PR have been removed, since they are no longer necessary.

Also of note:

  • Optional/IUO no longer has a custom mirror
  • I re-enabled the unit tests that were disabled because of the dependent bugs. However, I'm still working on making sure the bugs are truly gone and won't cause the tests to fail again.
  • There are a few changes to Reflection.mm, which should be looked over.
Contributor

austinzheng commented Jan 20, 2016

I pushed an update.

CGRect, CGPoint, and CGSize now conform to CustomDebugStringConvertible.

dump() now has a new technique for printing a description of the item being dumped, which is designed to mirror the old behavior:

  1. If the mirror display style is one of the following aggregate kinds, the description is in terms of number of children. (This relies on the idea that a collection's reflected children are synonymous with the items it contains.)
    • Collections are described like so: 5 elements
    • Dictionarys are described like so 5 key/value pairs
    • Sets are described like so: 5 members
    • Tuples are described like so: (5 elements)
  2. If the object conforms to CustomStringConvertible, CustomDebugStringConvertible, or Streamable, the object is allowed to describe itself.
  3. If the object does not conform and has a display style of Class, Struct, or Enum, the description is the fully-qualified type name + the enum case name (if applicable).
  4. Otherwise, the description is generated using _adHocPrint().

print(), debugPrint(), and all other APIs are unaffected by these changes.

I figured the work to make the above possible was simple enough that it didn't need to be discussed extensively before an initial implementation. If anyone thinks that the algorithm above should be changed, I'm happy to update the PR.

Unfortunately, the regression that @slavapestov mentioned in a line comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a description or a debugDescription, and it defaults to something like the mangled type name + address.

Many of the test changes that were in previous versions of this PR have been removed, since they are no longer necessary.

Also of note:

  • Optional/IUO no longer has a custom mirror
  • I re-enabled the unit tests that were disabled because of the dependent bugs. However, I'm still working on making sure the bugs are truly gone and won't cause the tests to fail again.
  • There are a few changes to Reflection.mm, which should be looked over.
@dabrahams

View changes

Show outdated Hide outdated stdlib/public/runtime/Reflection.mm Outdated
@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 20, 2016

Member

on Tue Jan 19 2016, Austin Zheng <notifications-AT-github.com> wrote:

Unfortunately, the regression that @slavapestov mentioned in a line
comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a
description or a debugDescription, and it defaults to something
like the mangled type name + address.

Through what path were we getting a sensible result in these cases
before? I don't see any mirrors or summary properties in that file...

Many of the test changes that were in previous versions of this PR
have been removed, since they are no longer necessary.

Also of note:

  • Optional/IUO no longer has a custom mirror

The less code, the mo' better.

  • I re-enabled the unit tests that were disabled because of the
    dependent bugs. However, I'm still working on making sure the bugs are
    truly gone and won't cause the tests to fail again.

Thanks for your diligence.

  • There are a few changes to Reflection.mm, which should be looked over.

Done, thanks!

Member

dabrahams commented Jan 20, 2016

on Tue Jan 19 2016, Austin Zheng <notifications-AT-github.com> wrote:

Unfortunately, the regression that @slavapestov mentioned in a line
comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a
description or a debugDescription, and it defaults to something
like the mangled type name + address.

Through what path were we getting a sensible result in these cases
before? I don't see any mirrors or summary properties in that file...

Many of the test changes that were in previous versions of this PR
have been removed, since they are no longer necessary.

Also of note:

  • Optional/IUO no longer has a custom mirror

The less code, the mo' better.

  • I re-enabled the unit tests that were disabled because of the
    dependent bugs. However, I'm still working on making sure the bugs are
    truly gone and won't cause the tests to fail again.

Thanks for your diligence.

  • There are a few changes to Reflection.mm, which should be looked over.

Done, thanks!

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 21, 2016

Contributor

I'll look into how NSObjects were printed and figure out how to replicate that behavior.

Unfortunately, although https://bugs.swift.org/browse/SR-426 has been fixed for the mini test case I wrote up, the unit test that prompted me to file it still fails. I'm going to have to dig into my new dump() tonight to figure out what is going on, and possibly file a new bug.

Contributor

austinzheng commented Jan 21, 2016

I'll look into how NSObjects were printed and figure out how to replicate that behavior.

Unfortunately, although https://bugs.swift.org/browse/SR-426 has been fixed for the mini test case I wrote up, the unit test that prompted me to file it still fails. I'm going to have to dig into my new dump() tonight to figure out what is going on, and possibly file a new bug.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 21, 2016

Contributor

I pushed another update. All unit and validation tests pass on OS X now.

Changes:

  • Rebased onto latest master.
  • Fixed a memory leak issue with printing the name of an enum case.
  • Changed the 'opaque summary' C++ function to return a const char *.
  • Minor improvement to dump(); the Mirror is no longer needlessly created twice (once in dump() itself, once in _dumpPrint()).
  • Removed some unused code that I had added in previously.

I also did some digging into the ugly Objective-C description. In the existing code, the dump() description is printed out using, roughly, _reflect(value).summary. When _reflect() is called on a class instance, it generates a _ClassMirror, whose summary property is just the type name.

The new dump() function tries to use one of description, debugDescription, or writeTo() to print out the dumped object's description, and only falls back to the type name if none of those exist.

Let me know if you have any questions.

Contributor

austinzheng commented Jan 21, 2016

I pushed another update. All unit and validation tests pass on OS X now.

Changes:

  • Rebased onto latest master.
  • Fixed a memory leak issue with printing the name of an enum case.
  • Changed the 'opaque summary' C++ function to return a const char *.
  • Minor improvement to dump(); the Mirror is no longer needlessly created twice (once in dump() itself, once in _dumpPrint()).
  • Removed some unused code that I had added in previously.

I also did some digging into the ugly Objective-C description. In the existing code, the dump() description is printed out using, roughly, _reflect(value).summary. When _reflect() is called on a class instance, it generates a _ClassMirror, whose summary property is just the type name.

The new dump() function tries to use one of description, debugDescription, or writeTo() to print out the dumped object's description, and only falls back to the type name if none of those exist.

Let me know if you have any questions.

@dabrahams

View changes

Show outdated Hide outdated stdlib/public/common/LeafMirrorConformance.gyb Outdated
[Runtime][StdLib] Migrate mirrors to use CustomReflectable API, rewri…
…te dump()

Jira: SR-88
Changes:
- Removed stdlib type conformances to _Reflectable
- Conformed stdlib types to CustomReflectable, CustomPlaygroundQuickLookable
- Rewrote dump() function to not use _reflect()
- CGRect, CGPoint, CGSize now conform to CustomDebugStringConvertible
- Rewrote unit tests for compatibility with new API
@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 21, 2016

Contributor

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest master. All tests continue to pass. I'll spin up a Ubuntu VM today and put Swift on it so I can run the suite there, wish me luck.

Let me know if you see any other issues or want any other changes!

Contributor

austinzheng commented Jan 21, 2016

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest master. All tests continue to pass. I'll spin up a Ubuntu VM today and put Swift on it so I can run the suite there, wish me luck.

Let me know if you see any other issues or want any other changes!

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 21, 2016

Member

on Thu Jan 21 2016, Austin Zheng <notifications-AT-github.com> wrote:

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest
master. All tests continue to pass. I'll spin up a Ubuntu VM today and
put Swift on it so I can run the suite there, wish me luck.

If we had public CI infrastructure I'd just merge it now, but since
you're willing to do this testing I think I'll wait as it reduces risk.
Please let me know how it turns out... and good luck!

Member

dabrahams commented Jan 21, 2016

on Thu Jan 21 2016, Austin Zheng <notifications-AT-github.com> wrote:

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest
master. All tests continue to pass. I'll spin up a Ubuntu VM today and
put Swift on it so I can run the suite there, wish me luck.

If we had public CI infrastructure I'd just merge it now, but since
you're willing to do this testing I think I'll wait as it reduces risk.
Please let me know how it turns out... and good luck!

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 22, 2016

Contributor

It looks like everything is nominal on Ubuntu:

screen shot 2016-01-21 at 6 57 42 pm

Contributor

austinzheng commented Jan 22, 2016

It looks like everything is nominal on Ubuntu:

screen shot 2016-01-21 at 6 57 42 pm

dabrahams added a commit that referenced this pull request Jan 22, 2016

Merge pull request #838 from austinzheng/az-port-mirror
Migrate mirrors to use CustomReflectable API, rewrite dump()

...and there was much rejoicing.

@dabrahams dabrahams merged commit d72d808 into apple:master Jan 22, 2016

@rudkx

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Jan 22, 2016

Member

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'

Member

rudkx commented Jan 22, 2016

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 22, 2016

Contributor

Thanks for bringing this to my attention. I'll try building tonight with the '-r' flag to troubleshoot the issue.

Austtin

Sent from my iPhone

On Jan 22, 2016, at 8:47 AM, Mark Lacey notifications@github.com wrote:

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'


Reply to this email directly or view it on GitHub.

Contributor

austinzheng commented Jan 22, 2016

Thanks for bringing this to my attention. I'll try building tonight with the '-r' flag to troubleshoot the issue.

Austtin

Sent from my iPhone

On Jan 22, 2016, at 8:47 AM, Mark Lacey notifications@github.com wrote:

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'


Reply to this email directly or view it on GitHub.

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 22, 2016

Contributor

I determined why it broke (although I still need to root cause it).

For some reason my master got out of sync with upstream's master. My branch's parent was my out-of-date master. One of the diffs between the upstream master and the out-of-date old master changed something that caused my code to become invalid.

I'll troubleshoot the exact cause and open a new PR tonight.

Contributor

austinzheng commented Jan 22, 2016

I determined why it broke (although I still need to root cause it).

For some reason my master got out of sync with upstream's master. My branch's parent was my out-of-date master. One of the diffs between the upstream master and the out-of-date old master changed something that caused my code to become invalid.

I'll troubleshoot the exact cause and open a new PR tonight.

@dabrahams

This comment has been minimized.

Show comment
Hide comment
@dabrahams

dabrahams Jan 22, 2016

Member

Thanks, Austin!

Member

dabrahams commented Jan 22, 2016

Thanks, Austin!

@rudkx

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Jan 23, 2016

Member

Awesome. I suspected something like that might have been the cause. Our commit rate is pretty high so it's not hard to end up in a situation like that (in fact it happened with a pair of commits today that came in at about the same time!).

Member

rudkx commented Jan 23, 2016

Awesome. I suspected something like that might have been the cause. Our commit rate is pretty high so it's not hard to end up in a situation like that (in fact it happened with a pair of commits today that came in at about the same time!).

@austinzheng

This comment has been minimized.

Show comment
Hide comment
@austinzheng

austinzheng Jan 23, 2016

Contributor

@rudkx Thanks for catching the issue and reverting the commit. I'll look into how I can make my workflow less prone to this sort of thing.

Contributor

austinzheng commented Jan 23, 2016

@rudkx Thanks for catching the issue and reverting the commit. I'll look into how I can make my workflow less prone to this sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment