Introduce special DeclNames for subscripts #6848

ahoppen · 2017-01-16T19:43:07Z

This is a preparation to use special DeclNames that don't carry identifiers e.g. for subscripts, initializers, and destructors which shall at some point fix SR-2575 as outlined in #5537 and #6305.

ahoppen · 2017-01-16T19:46:29Z

@jrose-apple: I've taken a more conservative approach now. Could you maybe have a quick look if this is going in the right direction? It's still kind of WIP and I haven't created the matching PR to the LLDB repo yet, so no need for running the test or doing an in-depth review. Thanks!

ahoppen · 2017-01-23T20:15:31Z

I'm making fairly good progress on this PR now and just have a few issues to fix. For that, I've got a quick question:

Subscripts are currently represented in SIL as <Type>.subscript!(getter|setter) which is the same spelling as for an instance variable named subscript. I.e. the accessor path for the following two is the same:

class Foo {
  subscript() -> String {
    return ""
  }
  
  var `subscript`: String
}

namely #Foo.subscript!getter.1

Until now this wasn't an issue since both of them were represented by the identifier Id_subscript so name lookup found either. Furthermore, the above code doesn't compile right now and complains about a redeclaration of subscript. However, with the introduction of special DeclNames, the above code is actually valid. So we would need a way to disambiguate the two in SIL to get the same expressive power there. My proposal is to rename the actual subscript reference in SIL to something like <Type>.[]!getter, [] being a constant string (nothing is meant to go in between the brackets), but I'm open to other solutions as well. The only requirement for the representation is that it isn't a valid identifier on the surface language.

Do you think this syntax would fit in the current spirit of SIL (which I really don't know a lot about) or is there another syntax coming to your mind that would be better? Thanks!

jrose-apple · 2017-01-27T21:45:44Z

Sorry for continuing to ignore this series, even after Branch Day—lots of 3.1 bugs to keep our hands full!

I'm no SIL expert, but I think expanding the set of allowed characters seems a bit iffy. Don't we have the same problem with init and deinit stepping on regular declarations? Maybe we should just add a leading '!' or something, i.e. !init and !subscript!getter. @jckarter, @swiftix, any thoughts on this particular issue?

jckarter · 2017-01-27T22:27:03Z

Or `subscript`!1 ?

ahoppen · 2017-01-28T16:56:51Z

Subscript in backticks sounds good. While implementing it, I found two issues that prevents generated SIL files from being parsed again. I just wanted to point to them (workarounds would be fine for me right now) since they make further development nearly impossible: SR-3773, SR-3774

ahoppen · 2017-02-04T19:15:36Z

Please test with following pull request:
apple/swift-lldb#106

@swift-ci Please test

swift-ci · 2017-02-04T19:16:02Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 12ce139b4fc5facf2d5b513368beea677db751e1
Test requested by - @ahoppen

swift-ci · 2017-02-04T19:17:01Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 12ce139b4fc5facf2d5b513368beea677db751e1
Test requested by - @ahoppen

ahoppen · 2017-02-05T10:49:01Z

Please test with following pull request:
apple/swift-lldb#106

@swift-ci Please test

swift-ci · 2017-02-05T10:49:31Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4f6855bd02d54c9c39c4e4f1a450ed604ea5e6ad
Test requested by - @ahoppen

swift-ci · 2017-02-05T10:49:57Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 4f6855bd02d54c9c39c4e4f1a450ed604ea5e6ad
Test requested by - @ahoppen

ahoppen · 2017-02-05T14:10:19Z

Hitting this failure: https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20170130/003954.html
Hoping a rerun solves the issue.

Please test with following pull request:
apple/swift-lldb#106

@swift-ci Please test

ahoppen · 2017-02-05T15:14:26Z

@jrose-apple: The PR is done so far from my side, could you (or someone else) review the changes?

jrose-apple

Thank you for continuing to push this forward, Alex. I find I'm still really uncomfortable with this change because it erases the distinction between operations that take compound names and those that only take base names (whether normal or special). I'm going to go back to saying that we need to change Identifier, not DeclName. @DougGregor?

jrose-apple · 2017-02-08T17:16:16Z

include/swift/AST/Decl.h

@@ -4698,6 +4700,8 @@ class AbstractFunctionDecl : public ValueDecl, public DeclContext {
  GenericEnvironment *getLazyGenericEnvironmentSlow() const;

 public:
+  Identifier getName() const { return getFullName().getBaseName(); }


This one I'd be happy to drop in favor of getBaseName(), but that can be a follow-up patch.

OK, I wanted to keep the code change as small as possible and not litter the PR with renames from getName to getBaseName.

jrose-apple · 2017-02-08T17:22:05Z

include/swift/AST/Identifier.h

+  StringRef str() const {
+    switch (getKind()) {
+    case DeclNameKind::Normal:
+      return getBaseName().str();


I don't think it's okay to drop the argument labels here. Either this should be called getBaseNameStr() (or something), or it should assert that this is not a compound name (and probably still have a less simple name than str()).

Renaming sounds like the way to go since IIRC this behaviour is needed in quite a few places.

jrose-apple · 2017-02-08T17:23:12Z

lib/AST/ASTMangler.cpp

@@ -1733,7 +1734,7 @@ void ASTMangler::appendProtocolConformance(const ProtocolConformance *conformanc
    appendIdentifier(
              fileUnit->getDiscriminatorForPrivateValue(behaviorStorage).str());
    appendProtocolName(conformance->getProtocol());
-    appendIdentifier(behaviorStorage->getName().str());
+    appendIdentifier(behaviorStorage->getBaseName().getBaseName().str());


Extra getBaseName here?

This basically asserts that the base name is not special because then the second getBaseName() will crash. Maybe an assert would indeed be cleaner.

jrose-apple · 2017-02-08T17:23:58Z

lib/AST/Decl.cpp

@@ -190,7 +190,7 @@ DescriptiveDeclKind Decl::getDescriptiveKind() const {
       return DescriptiveDeclKind::MaterializeForSet;
     }

-     if (!func->getName().empty() && func->getName().isOperator())
+     if (!func->getName().empty() && func->isOperator())


Does func->isOperator() already check for an empty name? In that case you could just use that.

It doesn't right now but it seems like a reasonable thing to add there.

jrose-apple · 2017-02-08T17:26:29Z

lib/AST/Decl.cpp

-                                      ArrayRef<Identifier>());
-
+          if (afd->getFullName().getKind() == DeclNameKind::Subscript) {
+            return DeclName::createSubscript(ctx, ArrayRef<Identifier>());


This isn't correct; you need the index labels too. Isn't it okay to just continue returning afd->getFullName() though?

I don't know why I changed this in the first place. I'll change it back to how it was

jrose-apple · 2017-02-08T18:02:38Z

lib/Sema/CSGen.cpp

@@ -1168,7 +1166,7 @@ namespace {
        CS.addBindOverloadConstraint(fnTy, choice, subscriptMemberLocator,
                                     CurDC);
      } else {
-        CS.addValueMemberConstraint(baseTy, Context.Id_subscript,
+        CS.addValueMemberConstraint(baseTy, DeclName::createSubscript(),


Does this correctly distinguish between subscript and subscript()?

I think so. I don't understand your specific concern though…

It's the difference between a reference to "any subscript" and "a subscript with zero arguments", or between a base DeclName and a compound DeclName. Just wanted to make sure this was producing a base DeclName and not a compound one.

jrose-apple · 2017-02-08T18:03:30Z

lib/Sema/CSSimplify.cpp

@@ -2795,8 +2795,16 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
    // anything else, because the cost of the general search is so
    // high.
    if (baseObjTy->isAnyObject() && argumentLabels) {
-      memberName = DeclName(TC.Context, memberName.getBaseName(),
-                            argumentLabels->Labels);
+      switch (memberName.getKind()) {


Maybe there should be a general withArgumentLabels operation on DeclName?

jrose-apple · 2017-02-08T18:05:11Z

lib/Sema/TypeCheckProtocol.cpp

+    }
+  } else {
+    if (lhs.getKind() == rhs.getKind()) {
+      score = 0;


What about argument labels?

Aren't they handled further down (line 5205 - 5210)?

Oh, whoops. I thought this was an early return for some reason.

jrose-apple · 2017-02-08T18:50:39Z

lib/Serialization/ModuleFile.cpp

@@ -297,22 +298,36 @@ namespace {
 /// Used to deserialize entries in the on-disk decl hash table.
 class ModuleFile::DeclTableInfo {
 public:
-  using internal_key_type = StringRef;
-  using external_key_type = Identifier;
+  using internal_key_type = std::pair<DeclNameKind, StringRef>;


This is good, use something like this for the ClangImporter lookup tables, please.

jrose-apple · 2017-02-08T18:51:35Z

lib/Serialization/Deserialization.cpp

+    // Subscript
+    return DeclName::createSubscript();
+  } else {
+    // Serialized prior to special DeclNames. Just read the normal identifier


At this point the module format isn't stable, so we can just make a breaking change here. It seems more important to look forward than backward; in a forward world, we've introduced a new special kind that this compiler treats as a normal name. We could either import it as a normal name or fail.

OK, I didn't want to be the first one to increase the major version though ;-) But with your OK, I'll go ahead ;-)

We're treating the minor version as a breaking version for now, please continue doing that.

ahoppen · 2017-02-09T12:33:36Z

Thanks for the review. It explained a few things I didn't quite understand before. 😄
I'll wait with any further changes until we've reached a decision whether this is the way forward or not. If we decide to go a more Identifier centric approach, there is still my initial idea of using $subscript similar to $match in switch statements, which we could revive again.

ahoppen · 2017-03-02T20:25:27Z

Just a friendly ping that it would be great if you could discuss internally if we want to continue with the DeclName based approach or try to model the special names using Identifier. I really don't feel like I'm able to judge what's best in the overall picture.

rjmccall · 2017-03-02T20:43:56Z

It sounds to me like the base name of a DeclName should use a new DeclBaseName type that's either an identifier or one of these special names. We shouldn't contaminate the entire Identifier type with the possibility of special names, but we also shouldn't pretend that special names only make sense at the DeclName level.

ahoppen · 2017-03-02T20:47:59Z

Sounds very reasonable to me.
And I guess then change some of the places where we currently (prior to this PR) use Identifiers to use the new DeclBaseName if it could be a special name.

rjmccall · 2017-03-02T20:54:59Z

Yeah. That might not be a very fun project, but it's probably the right thing to do. I'd wait for @jrose-apple's blessing before getting started, though.

jrose-apple · 2017-03-03T02:10:08Z

I'm sad to have three types, because I'm not sure Identifier will be pulling its weight after this, but it's always easier to collapse back down to two if we change our minds later, and right now we really have identified three different kinds of entities.

jrose-apple · 2017-03-03T02:10:24Z

Thanks for weighing in, @rjmccall.

ahoppen · 2017-03-03T15:49:04Z

Cool, I'll try and tackle this one more time then ;-)

The new DeclBaseName type will later abstract over normal Identifiers as they exist now and special names that don't have an identifier in the surface language like subscripts

Special names will in the future represent names that don't have a surface language identifier like subscript. For this, change the backing type of DeclName to a new type DeclBaseName that will later be able to represent special names. ValueDecls may therefore no longer have an identifier name. Thus we move the getName method from ValueDecl to the subtypes that are guaranteed to have identifier names

Special DeclNames represent names that do not have an identifier in the surface language. This implies serializing the information about whether a name is special together with its identifier (if it is not special) in both the module file and the swift lookup table. ValueDecls are not guaranteed to have a name anymore and thus getNameStr is moved from ValueDecl to subclasses that guarantee to be backed by identifiers. Other calls need to handle special names if the context doesn't guarantee that the declaration's name is non-special.

ahoppen · 2017-03-05T19:58:10Z

Please test with following pull request:
apple/swift-lldb#165

@swift-ci Please test

swift-ci · 2017-03-05T19:59:24Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 87bfcd8cf560b45c61888a529417c08b8615bfc6
Test requested by - @ahoppen

swift-ci · 2017-03-05T19:59:52Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 87bfcd8cf560b45c61888a529417c08b8615bfc6
Test requested by - @ahoppen

ahoppen · 2017-03-06T07:00:54Z

OK, so I've implemented the changes with a new type DeclBaseName now. I personally feel like this could be a good solution, the impact of the changes seems to be far lower than with the other approaches. So, a review would be very welcome.

I've got one more open point which is that subscripts are currently mangled as something like _TFC4main3Foog9subscriptFT_Si. I feel like we shouldn't include the string 'subscript' in the mangled name anymore, but that's something to be tackled in a subsequent PR, I'd say.

jrose-apple

This does look a lot better. It seems obvious now that we really do have three different kinds of things. Thanks, Alex!

The patch is still pretty big; it would be nice to factor out some of the changes that don't depend on DeclBaseName already existing, so we can review those separately and/or get those in first. I scanned through the entire thing today but that doesn't mean I didn't miss something.

jrose-apple · 2017-03-08T17:09:42Z

include/swift/AST/Identifier.h

+    return userFacingStr().compare(other.userFacingStr());
+  }
+
+  bool operator==(StringRef Str) const {


These seem too easy to misuse. Maybe we should go the Clang route: bool isStr(StringRef str).

What kind of misuse do you have in mind? I quite liked the brevity of == in ifs. But I can, of course, change it

I guess I was worried about == "subscript" (and whatever else we add), and maybe that's being too cautious.

jrose-apple · 2017-03-08T17:10:58Z

include/swift/AST/Identifier.h

+
+raw_ostream &operator<<(raw_ostream &OS, swift::DeclBaseName D);
+
+// Identifiers hash just like pointers.


Nitpick: comment.

jrose-apple · 2017-03-08T17:11:59Z

include/swift/AST/Identifier.h

+// Identifiers hash just like pointers.
+template<> struct DenseMapInfo<swift::DeclBaseName> {
+  static swift::DeclBaseName getEmptyKey() {
+    return swift::DeclBaseName::getEmptyKey();


Nitpick: Why the extra level of indirection? It's not like getEmptyKey is going to be used anywhere else.

(I see that Identifier did this too, but it's weird there too.)

I was just copying what Identifier did ;-). I'll change it.

jrose-apple · 2017-03-08T17:17:40Z

include/swift/AST/Mangle.h

@@ -154,6 +154,8 @@ class Mangler {
  void mangleProtocolList(ArrayRef<Type> protocols);
  void mangleIdentifier(Identifier ident,
                        OperatorFixity fixity = OperatorFixity::NotOperator);
+  void mangleIdentifier(DeclBaseName ident,


What do we want to do about things like this? Is continuing to say mangleIdentifier the right way forward? I think it's fine for this patch (minimizes an already large changeset), but what do you think for follow-up?

I would imagine that we will at some point change the mangling name of subscripts to something that doesn't contain the string "subscript" (I'd use a new special character in the mangling scheme). Then the special names would be handled on the call side of this function and we could remove it again.

Ah, right, we could do it based on the DeclKind. Okay, that seems reasonable.

jrose-apple · 2017-03-08T17:20:11Z

include/swift/Serialization/ModuleFormat.h

+// have an identifier. As above, these IDs must \em not be renumbered or
+// reordered without incrementing VERSION_MAJOR.
+enum SpecialNameID : uint8_t {
+  SUBSCRIPT_ID = NUM_SPECIAL_MODULES,


This seems dangerous. How do people know whether to check NUM_SPECIAL_MODULES or NUM_SPECIAL_IDS? I think we should either fold the two lists together or allow the same value to represent multiple things (builtin module OR subscript, current module OR init, etc).

Yeah, I think folding the two enums together would be the right way to go. Didn't really feel too comfortable with it but wanted an opinion on whether using the same enum for both special module names and other special names would be alright.

jrose-apple · 2017-03-08T17:58:00Z

test/Sema/subscripting.swift

@@ -0,0 +1,16 @@
+// RUN: %target-run-simple-swift | %FileCheck %s


Nitpick: execution tests don't belong in test/Sema/. Can you move this to test/Interpreter/ instead?

jrose-apple · 2017-03-08T17:58:57Z

test/decl/subscript/subscripting.swift

+  }
+}
+
+SR2575().subscript() // expected-error{{type 'SR2575' has no member 'subscript'}}


We should probably make a note / bug to come back and improve this specific case later.

What do you wan to change here? Offer a fixit to change the call to subscript to a proper subscript?

Something like that. Maybe something that makes it clear it's looking for functions named subscript rather than actual subscripts.

OK, I'll open a follow-up JIRA once the PR has been landed.

jrose-apple · 2017-03-08T18:01:41Z

test/expr/unary/selector/selector.swift

@@ -116,7 +116,7 @@ func testParseErrors3(_ c1: C1) {

 func testParseErrors4() {
  // Subscripts
-  _ = #selector(C1.subscript) // expected-error{{type 'C1.Type' has no subscript members}}
+  _ = #selector(C1.subscript) // expected-error{{type 'C1' has no member 'subscript'}}


I think we are going to want to support syntax like this for getting the selector of a subscript getter or setter some day: #selector(getter:C1.subscript). It doesn't currently work but that doesn't mean it shouldn't work.

I'd say that C1.subscript will always be invalid unless you declare a func `subscript`() {}. Otherwise we would get into ambiguity issues if you declare both a subscript and a function named subscript.

That's what the getter: is for. Or whatever the no-argument function disambiguation syntax turns out to be.

Sorry, but then I don't get why we want to support syntax like this. #selector(getter:…) would then need to implement some special lookup logic to handle the subscript case, I'd say.

…and I don't quite understand your objection. How do you want to spell "the selector of the getter for this subscript"?

I suppose we haven't had a huge need for it yet, but it's not an unreasonable thing to ask for.

I think my point is that, from my point of view, whether or not we want to support selectors for subscripts is nothing to worry about for now and that we will only need to worry about how to implement it when the time comes.
Or maybe let's ask the other way round: What would you want to be changed here?

I'm concerned about the diagnostic. It implies that the compiler is interpreting this syntax as a reference to a member named 'subscript', which would lead to a user thinking they've done something wrong, rather than this simply being a feature that's not implemented yet.

It's not a deal-breaker, but I consider us not supporting subscript selectors to just be a bug, and therefore the compiler shouldn't tell the user they've done something wrong.

Ah, OK, now I see. For now I'd suggest changing the error message to type 'C1' has no member named 'subscript' (adding the word 'named'). I guess then it's correct since C1 doesn't have a member named subscript but only a subscript-member.

In a subsequent PR we could then maybe improve the diagnostics by looking if the type would have a subscript-member (which was probably meant) and point out that selectors for subscripts are not yet supported. Once we do support them, this would turn into a fixit for the getter or setter syntax.

jrose-apple · 2017-03-08T18:02:26Z

tools/swift-api-digester/swift-api-digester.cpp

+  case DeclBaseName::Kind::Subscript:
+    return "subscript";
+  case DeclBaseName::Kind::Normal:
+    return llvm::StringSwitch<StringRef>(name.getIdentifier().str())


Rather than a StringSwitch, we should just escape anything that's a keyword.

I don't want to escape all keywords here. A function named default should still be output unescaped. I'm basically escaping a new class of keywords here, namely those that represent special names (in which "subscript" is currently the only member)

I don't think that makes sense. You wouldn't want to see func func in the output of this tool. @nkcsgexi, what do you think? (Sorry to pull you in mid-discusson.)

I think for swift-api-digester per se, being consistent is more important than being precise. So this change is fine as long as we always keep subscript escaped; however, this may not help the tool itself much.

jrose-apple · 2017-03-08T18:03:15Z

validation-test/Sema/type_checker_crashers_fixed/rdar27017206.swift

@@ -1,4 +1,4 @@
-// RUN: not --crash %target-swift-frontend %s -typecheck
+// RUN: not %target-swift-frontend %s -typecheck
 // REQUIRES: asserts


Please drop the "REQUIRES: asserts" as well. (We're not good about this and often clean them up in large batches, but it's good to get in the habit.)

ahoppen · 2017-03-09T08:46:12Z

Thanks for the review.

I guess the only way I could split this in some sensible way would be across my different commits. This would mean, we could split it in up to 5 pull requests:

Change calls of getName().isOperator() to isOperator()
- Really small patch
Change the backing base name type of DeclName to DeclBaseName
- Introduce DeclBaseName as an implementation detail of DeclName but don't expose it yet
- Basically only affects Identifier.h
Preparations for special names
- Expose DeclBaseName to the project but without special names for the moment
- This basically adds a bunch of todos in places where special names will be handled later
Introduce special decl names
- Actually introduce special names and remove the todos added in the previous commit
- This and the above commit are both still fairly big (~500 lines) but I can't think of any sensible way to break them apart so that it still compiles in between
Add test cases for special subscript name
- Should probably be merged together with the previous commit

jrose-apple · 2017-03-09T17:53:28Z

That seems like a reasonable way to split it up. If (3) and (4) don't split nicely at least we'd have gotten (1) and (2) out of the way first, so they're not intermingled with everything else.

ahoppen · 2017-03-10T09:11:20Z

I'll create PRs one by one now, splitting (3) and (4) since the commits are already separate. We can always merge them again if we want to.

Part 1 -> #8015

ahoppen requested a review from jrose-apple January 16, 2017 19:43

ahoppen changed the title ~~Sr 2575 3 identifier to decl name~~ WIP: Use DeclName instead of Identifier in various locations again Jan 16, 2017

ahoppen mentioned this pull request Jan 16, 2017

Use DeclName instead of Identifier in various locations #6305

Closed

ahoppen force-pushed the SR-2575-3-Identifier-toDeclName branch 2 times, most recently from 34ed74c to 4229f92 Compare January 24, 2017 11:15

ahoppen force-pushed the SR-2575-3-Identifier-toDeclName branch 3 times, most recently from 8049fbc to 4f6855b Compare February 4, 2017 19:12

ahoppen force-pushed the SR-2575-3-Identifier-toDeclName branch from 4f6855b to 87bfcd8 Compare February 5, 2017 10:48

ahoppen changed the title ~~WIP: Use DeclName instead of Identifier in various locations again~~ Introduce special DeclNames for subscripts Feb 5, 2017

jrose-apple reviewed Feb 8, 2017

View reviewed changes

ahoppen force-pushed the SR-2575-3-Identifier-toDeclName branch 2 times, most recently from 8a08dab to 2a3e8ad Compare March 5, 2017 19:43

ahoppen added 5 commits March 5, 2017 19:51

[Gardening] Change calls of getName().isOperator() to isOperator()

d7892e8

Change the backing base name type of DeclName to DeclBaseName

c678b96

The new DeclBaseName type will later abstract over normal Identifiers as they exist now and special names that don't have an identifier in the surface language like subscripts

Add test cases for special subscript name

b5074d3

ahoppen force-pushed the SR-2575-3-Identifier-toDeclName branch from 2a3e8ad to b5074d3 Compare March 5, 2017 19:52

ahoppen mentioned this pull request Mar 5, 2017

Adopt changes for special names in the swift repository apple/swift-lldb#165

Closed

jrose-apple reviewed Mar 8, 2017

View reviewed changes

ahoppen mentioned this pull request Mar 10, 2017

[Gardening] Change calls of getName().isOperator() to isOperator() #8015

Merged

This was referenced Mar 10, 2017

Change the backing base name type of DeclName to DeclBaseName #8019

Merged

Preparations for special names #8053

Closed

ahoppen closed this Jun 26, 2017

ahoppen deleted the SR-2575-3-Identifier-toDeclName branch June 26, 2017 16:53

ahoppen mentioned this pull request Jul 1, 2017

Introduce special decl names #9989

Merged

ahoppen mentioned this pull request Apr 25, 2022

[SR-4167] swift-ci posting “Build failed” message when starting run #46750

Closed


		raw_ostream &operator<<(raw_ostream &OS, swift::DeclBaseName D);

		// Identifiers hash just like pointers.

		@@ -0,0 +1,16 @@
		// RUN: %target-run-simple-swift \| %FileCheck %s

Introduce special DeclNames for subscripts #6848

Introduce special DeclNames for subscripts #6848

Uh oh!

Conversation

ahoppen commented Jan 16, 2017

Uh oh!

ahoppen commented Jan 16, 2017

Uh oh!

ahoppen commented Jan 23, 2017

Uh oh!

jrose-apple commented Jan 27, 2017

Uh oh!

jckarter commented Jan 27, 2017

Uh oh!

ahoppen commented Jan 28, 2017

Uh oh!

ahoppen commented Feb 4, 2017

Uh oh!

swift-ci commented Feb 4, 2017

Uh oh!

swift-ci commented Feb 4, 2017

Uh oh!

ahoppen commented Feb 5, 2017

Uh oh!

swift-ci commented Feb 5, 2017

Uh oh!

swift-ci commented Feb 5, 2017

Uh oh!

ahoppen commented Feb 5, 2017

Uh oh!

ahoppen commented Feb 5, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Feb 9, 2017

Uh oh!

ahoppen commented Mar 2, 2017

jrose-apple Mar 8, 2017 •

edited

Loading

jrose-apple Mar 9, 2017 •

edited

Loading