Wire up FileSpecificDiagnosticConsumer for serialized diagnostics #14900

jrose-apple · 2018-03-01T02:31:44Z

Now that most of @davidungar's batch mode work has landed in the frontend, I can actually use the RangeSpecificDiagnosticConsumer from #14844 to get us outputting multiple .dia files in batch mode. We want diagnostics that are definitely associated with a particular file to be emitted in that file's .dia output, while diagnostics that may be associated with other files (such as those that come from Clang) get emitted in all .dia outputs to make sure they show up everywhere they're needed.

This deserves some extra documentation somewhere about why this is safe and correct, but hey, it appears to work!

jrose-apple · 2018-03-01T02:31:50Z

@swift-ci Please test

swift-ci · 2018-03-01T02:53:41Z

Build failed
Swift Test Linux Platform
Git Sha - 3d345a8260cee4834abef9c88020a7e7e47ebc33

swift-ci · 2018-03-01T03:48:29Z

Build failed
Swift Test OS X Platform
Git Sha - 3d345a8260cee4834abef9c88020a7e7e47ebc33

davidungar · 2018-03-01T05:19:18Z

include/swift/AST/DiagnosticConsumer.h

+/// \brief DiagnosticConsumer that funnels diagnostics in certain files to
+/// particular sub-consumers.
+///
+/// Diagnostics that are not in one of the special files are emitted into every


Adding a sentence or two to explain what a "certain" or "special" file is would help a novice to understand what's going on here. What's the basic problem? Something like: Xcode needs serialized diagnostic files with the diagnostic routed to the right file. [Why?] This bigger picture is the sort of thing one cannot get from just reading the code.

I was doing that somewhat intentionally, since this is in theory a reusable component. But you're right, a few lines on the example, intended use would make sense.

davidungar · 2018-03-01T05:21:20Z

include/swift/AST/DiagnosticConsumer.h

+  /// position can be done using binary search.
+  ///
+  /// \see #consumerForLocation
+  SmallVector<std::pair<CharSourceRange, unsigned>, 4> LocationToConsumerMap;


Or you could call it something like ConsumersOrderedByLocation which is a tad more specific.

I had to read consumerForLocation to figure out that LocationToConsumerMap maps not directly to a consumer but to an index in the SubConsumers vector. Suggest adding a comment to point out or at least imply the invariant that you cannot later change the code to mess with the order in that vector without also adjusting the map.

To the second comment: I should probably just make the SubConsumers vector const.

To the first: Hm, I'll think about it. I don't quite like that name either but it is better than what I have, now that you've pointed it out.

davidungar · 2018-03-01T05:21:36Z

include/swift/AST/DiagnosticConsumer.h

+  /// that was most recently emitted.
+  ///
+  /// If null, Note diagnostics are sent to every consumer.
+  DiagnosticConsumer *ConsumerForSubsequentNotes = nullptr;


Yeah, I realized I was being too clever. I think at some point we're going to want to emit notes before a particular error or warning (which Clang already does in some cases, IIRC), and then we're going to need the notion of an explicit "diagnostic group", rather than the implicit thing we have now. But we can change that when it comes to that.

davidungar · 2018-03-01T05:24:14Z

include/swift/AST/DiagnosticConsumer.h

+public:
+  /// Takes ownership of the DiagnosticConsumers specified in \p consumers.
+  ///
+  /// There must be no duplicate files, but an empty file name can be used to


Because this sentence uses both "duplicate files" and "file name" it could potentially confuse a reader. How about "File paths must be unique, but an empty file path ..."? Crazy question: Can an empty file path be duplicated? Does an empty string really stand for no-path? This convention has confused me before: I think the code base uses empty strings in many places to mean "no string". My preference would be to explicitly call such a convention out in a comment, even though my code has neglected to do so, because it seemed like common practice.

davidungar · 2018-03-01T05:31:04Z

lib/AST/DiagnosticConsumer.cpp

+FileSpecificDiagnosticConsumer::FileSpecificDiagnosticConsumer(
+    SmallVectorImpl<ConsumerPair> &consumers)
+  : SubConsumers(std::move(consumers)) {
+#ifndef NDEBUG


I had to stare at this for a minute to figure out that the only purpose for the following code here is to check for duplicate files. If it had been factored out into a function called something like checkForDuplicateFiles it would have sped up my comprehension.

davidungar · 2018-03-01T05:32:29Z

lib/AST/DiagnosticConsumer.cpp

+#ifndef NDEBUG
+  llvm::StringSet<> seenFiles;
+  for (const ConsumerPair &consumerPair : SubConsumers) {
+    if (consumerPair.first.empty())


When would the first element ever be empty? Is it worth a comment somewhere?

That's the "empty files represent no affinity" thing from the doc comment of the constructor.

Hmm... Maybe reify that into a (static) function that returns a bool, and takes a ConsumerPair argument, and call the function hasAffinity.

davidungar · 2018-03-01T05:33:06Z

lib/AST/DiagnosticConsumer.cpp

+    if (consumerPair.first.empty())
+      continue;
+    bool isUnique = seenFiles.insert(consumerPair.first).second;
+    assert(isUnique &&


Why would duplicates be an error instead of just something to ignore?

Because the way things are set up only one of the two consumers will get the diagnostics from that file, rather than both of them, and I don't think it's even guaranteed which (because of std::sort). We could change that if it ever mattered, but it's slightly harder to implement and we don't need it today.

Add what you wrote above as a comment for the assert?

davidungar · 2018-03-01T05:35:15Z

lib/AST/DiagnosticConsumer.cpp

+}
+
+void
+FileSpecificDiagnosticConsumer::buildLocationToConsumerMap(SourceManager &SM) {


Factoring this out really helps clarify the code for me.

davidungar · 2018-03-01T05:43:33Z

include/swift/AST/DiagnosticConsumer.h

@@ -111,6 +112,55 @@ class NullDiagnosticConsumer : public DiagnosticConsumer {
                        ArrayRef<DiagnosticArgument> FormatArgs,
                        const DiagnosticInfo &Info) override;
 };
+
+/// \brief DiagnosticConsumer that funnels diagnostics in certain files to
+/// particular sub-consumers.


Why is the funneling needed? (I know I already commented about a bigger picture comment, which could help.)
Inheritance, or aggregation? Is it a special kind of DiagnosticConsumer that funnels, or is it an aggregation or ordinary DiagnosticConsumers, a multiplexer in front of a bunch of them? Just asking, don't know the best answer.

davidungar · 2018-03-01T05:46:47Z

lib/AST/DiagnosticConsumer.cpp

+                   getRawLoc(right.first.getEnd()).getPointer());
+  });
+
+  // Check that the ranges are non-overlapping.


What part of the compiler guarantees non-overlap? What would be the harm? Would this code fall over and die, or just possibly misfile a diagnostic? If in the future, someone introduces a bug that produces overlapping ranges, do we want the compiler to crash or to merely whine?

Misfiled diagnostics, but yes, I think we still want the compiler to crash in such a case when assertions are enabled.

davidungar · 2018-03-01T05:47:29Z

lib/AST/DiagnosticConsumer.cpp

+
+  // This map is generated on first use and cached, to allow the
+  // FileSpecificDiagnosticConsumer to be set up before the source files are
+  // actually loaded.


Great comment!

davidungar · 2018-03-01T05:47:57Z

lib/AST/DiagnosticConsumer.cpp

+  // actually loaded.
+  if (LocationToConsumerMap.empty()) {
+    auto *mutableThis = const_cast<FileSpecificDiagnosticConsumer*>(this);
+    mutableThis->buildLocationToConsumerMap(SM);


Love the use of mutableThis!

davidungar · 2018-03-01T05:49:18Z

lib/AST/DiagnosticConsumer.cpp

+  }
+
+  // "Find the first range whose end location is greater than or equal to
+  // 'loc'."


Are the quotes around the comment a Doxygen thing?

I was trying to imply that it was a sort of "translation" of the following invocation, rather than a higher-level operation. (Because as a higher-level operation, this would be a terrible comment. It doesn't tell you why it's looking for something >=.)

That didn't come across, so I'll think about rephrasing.

Another way to do that could be to use a verbose name for possiblyContainingRangeIter such as locatorForEncompassingRange.

That's the same name, no? The iterator for the range that might contain the location.

davidungar · 2018-03-01T05:51:30Z

lib/AST/DiagnosticConsumer.cpp

+                   getRawLoc(loc).getPointer());
+  });
+
+  if (possiblyContainingRangeIter != LocationToConsumerMap.end() &&


My style would be to factor out this predicate into a function called actuallyContains or some such.

Since it takes all the context of the current method (both the LocationToConsumerMap and the location we're searching for), I don't think that pulls its weight. Doubly so because iterators are weird annoying types to spell out.

Was it clear that I meant merely factoring out possiblyContainingRangeIter != LocationToConsumerMap.end() & possiblyContainingRangeIter->first.contains(loc)? Let's see... you would have to pass in possiblyContainingRangeIterandLocationToConsumerMap.endandloc. I would still factor it out, but I see your point! If it doesn't pull its weight, how about a comment for the if` like: "Does it really contain the location?".

I think the new comment at the top of the lower_bound covers this now.

davidungar · 2018-03-01T05:55:22Z

lib/AST/DiagnosticConsumer.cpp

+    StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
+    const DiagnosticInfo &Info) {
+
+  DiagnosticConsumer *specificConsumer;


I would factor the switch out into a function that returned the specificConsumer (or null). The specificConsumer variable becomes local to the first three cases that way, so it's less code to trace through in a reader's head.

I still don't think that's worth it, especially since there's an update to ConsumerForSubsequentNotes. Lots of small functions are also hard to reason about.

Yeah, there's a bit of Smalltalk-oidal transformative experience going on on my part here. Here's how I would push back: In this case, it would be a function called findSpecificConsumerIfAny and I could just read the name and know what's going on in the caller. Without the separate function, I need to read the body of the switch to see that it is setting the variable, and then also read further to see that the variable is never changed again. (Swift is way better:)

I realize that the uninitialized variable followed by conditional code that sets it is a common idiom in C(++), but my other beef is that the compiler does nothing to guarantee it is not modified below. When functions get longer, I always have to grep for the variable to ensure it doesn't get modified in the middle.

This particular case is not that bad, but I feel that there is something like "the death of a thousand cuts", where enough not-too-tough cases have a cumulative effect on the effort required to dive into an unfamiliar code base.

davidungar · 2018-03-01T05:57:33Z

lib/AST/DiagnosticConsumer.cpp

+  // behavior.
+  bool hadError = false;
+  for (auto &subConsumer : SubConsumers)
+    hadError |= subConsumer.second->finishProcessing();


I have seen this idiom before in our code, but I worry about the bool/int conflation.

It's always safe with C++ bool, so I don't feel too bad about it.

davidungar · 2018-03-01T06:02:08Z

lib/FrontendTool/FrontendTool.cpp

-  // FIXME: Do we need one SerializedConsumer per primary? Owned by the
-  // SourceFile?
-  std::unique_ptr<DiagnosticConsumer> SerializedConsumer;
+  std::unique_ptr<DiagnosticConsumer> SerializedConsumerMultiplexer;


SerializedConsumerMultiplexer gives me pause. Thinking aloud... It directs SerializedDiagnostics to the appropriate consumer. In a way, it's a demultiplexer, taking a single stream of multiplexed diagnostics and separating them out into separate streams, each directed to a specific consumer. Feels like there could be a better name here.

Oops, right. I always get the mux/demux order backwards.

I don't actually care what this name is, and neither does the code; this is only here to keep the consumer alive. I had TopSerializedConsumer before but that was really even more vacuous. Any suggestions?

SerializedDiagnosticConsumerSorter? SerializedDiagnosticDistributor?? What's this thing doing? Directing diagnostics to an ultimate consumer?? DiagnosticPachinko? (No, that would be random.)

Pavel came up with "dispatcher", which I thought was fine. "Distributor" isn't bad either.

davidungar · 2018-03-01T06:05:11Z

lib/FrontendTool/FrontendTool.cpp

+      std::string SerializedDiagnosticsPath = Input.serializedDiagnosticsPath();
+      if (SerializedDiagnosticsPath.empty()) {
+        // If one input does not have a serialized diagnostic output path, none
+        // of them will.


Is this checked somewhere with a diagnostic issued? Does it need to be?

For noobs could add the following to the comment: "because serialized diagnostics are only used when Swift is invoked for Xcode, and in that case each primary input must communicate its diagnostics back to the IDE."

Uh, hm. I was assuming this was guaranteed by the driver, not the IDE. I suppose there's no reason to build in that limitation here, though. I'll just return false instead of true to keep going.

davidungar · 2018-03-01T06:08:39Z

lib/FrontendTool/FrontendTool.cpp

-        Invocation.getSerializedDiagnosticsPathForAtMostOnePrimary();
-    if (!SerializedDiagnosticsPath.empty()) {
-      SerializedConsumer.reset(
+    SmallVector<FileSpecificDiagnosticConsumer::ConsumerPair, 4>


Why 4? There must be something about the implementation of SmallVector. If I had to guess, I would bet that the common cases are 1 and many, FWIW.

After reading it, I see that the following code produces a vector of ConsumerPairs. Suggest adding either a comment to that effect or factoring into a subroutine. In any event, it would help to add a comment explaining why such a vector is needed.

I think you're right about "1 and many", but there are lots of projects where "many" is still going to be <= 4. (On an eight-core machine, that supports up to 32 files in a clean build.) You're right that I just made the number up, though.

I don't understand what you mean about the following code. The vector is built up and then passed to the constructor that uses it. The code that does the building up says what it does. What would the subroutine be called?

(I could see factoring out the whole creation of the FileSpecificDiagnosticConsumer, including building this vector, but just the vector part doesn't seem to buy us anything.)

Regarding the 4, how about a comment explaining why 4?

Regarding factoring the vector-building: lines 1627 - 1653 are all computing one thing, the SerializedConsumerMultiplexer. But I had to read it carefully to see that that was the only thing it was doing, and that the multiplexer is not modified thereafter. Likewise for the 20-ish lines to compute the vector. It feels like there are functions being computed, but it could be easier to see that than what is apparent at a glance.

Re: vector-building: Again, I'd be more inclined to pull out the whole multiplexer part (now "dispatcher" thanks to Pavel). I can do that. If I wanted to pin down why the vector computation isn't worth pulling out separately, I think it's because it's not reusable in any interesting way and it can only be described in terms of "this will then be used to create a FileSpecificDiagnosticConsumer".

Re: 4: I don't know. We have SmallVectors all over the place. Most of them have arbitrary initial sizes. 4 sounds good to me, but is it better than 8? I have no idea. I don't think it's worth my time to look into, but maybe it is.

davidungar · 2018-03-01T06:14:29Z

lib/FrontendTool/FrontendTool.cpp

-      SerializedConsumer.reset(
+    SmallVector<FileSpecificDiagnosticConsumer::ConsumerPair, 4>
+      SerializedConsumers;
+    Invocation.getFrontendOptions().InputsAndOutputs


Is it good practice to add a (void) in front of this line to show that the code explicitly throws away the result?

Hm. Maybe? I feel like this is one of those cases where the result was always incidental to begin with, like the result of Dictionary.remove in Swift. Some situations care about it, some don't, but ignoring it is intended much more often than it's a mistake.

OK. Whatever the common practice is for our team is OK with me.

davidungar · 2018-03-01T06:17:45Z

lib/FrontendTool/FrontendTool.cpp

+      return false;
+    });
+
+    if (!SerializedConsumers.empty()) {


It is possible to execute this block even if SerializedConsumers is empty? Would that just use a bit more time? It could potentially make the compiler a scintilla more reliable.

It would waste time on every diagnostic. I'd rather not.

Is there a conceivable situation where the time difference would be noticeable? I had assumed that the extra time for a diagnostic (when there are no serialized consumers) would be unobservable, given the time required to generate the diagnostic, but maybe I'm wrong. It would be good for me to correct my intuition.

If the if-test is merely an optimization, and not needed for correctness, it would help to add a comment: "Save time when there are no SerializedConsumers" so that future maintainers know it is just an optimization.

It probably is negligible, but that just feels like the wrong attitude to me. The test is free and doesn't really add any complexity for a human, and it makes the implementation of FileSpecificDiagnosticConsumer have one less case to worry about. (Right now it would try to build the sorted list of consumers over and over again because I happen to be using "empty" as a sentinel state for "not yet computed".)

davidungar

Looks very good! I've noted places where it might be possible to increase the clarity of the code.

jrose-apple · 2018-03-02T00:05:31Z

New version, with some but not all of David's suggestions.

@swift-ci Please test

swift-ci · 2018-03-02T00:05:55Z

Build failed
Swift Test OS X Platform
Git Sha - 3d345a8260cee4834abef9c88020a7e7e47ebc33

swift-ci · 2018-03-02T00:07:04Z

Build failed
Swift Test Linux Platform
Git Sha - 3d345a8260cee4834abef9c88020a7e7e47ebc33

jrose-apple · 2018-03-02T00:08:36Z

lib/FrontendTool/FrontendTool.cpp

    }
  }

+  // FIXME: Do the fix-its need to be multiplexed like the serialized
+  // diagnostics?


I talked to Xi and the answer is "yes", but I'll do that in a separate PR.

davidungar

At this point, I don't have anything more to contribute, and the substance is fine. Land it or polish it, either is OK.

davidungar · 2018-03-01T18:18:58Z

lib/AST/DiagnosticConsumer.cpp

+  }
+
+  // "Find the first range whose end location is greater than or equal to
+  // 'loc'."


Another way to do that could be to use a verbose name for possiblyContainingRangeIter such as locatorForEncompassingRange.

davidungar · 2018-03-01T18:24:35Z

lib/AST/DiagnosticConsumer.cpp

+                   getRawLoc(loc).getPointer());
+  });
+
+  if (possiblyContainingRangeIter != LocationToConsumerMap.end() &&


Was it clear that I meant merely factoring out possiblyContainingRangeIter != LocationToConsumerMap.end() & possiblyContainingRangeIter->first.contains(loc)? Let's see... you would have to pass in possiblyContainingRangeIterandLocationToConsumerMap.endandloc. I would still factor it out, but I see your point! If it doesn't pull its weight, how about a comment for the if` like: "Does it really contain the location?".

davidungar · 2018-03-01T18:30:49Z

lib/AST/DiagnosticConsumer.cpp

+    StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
+    const DiagnosticInfo &Info) {
+
+  DiagnosticConsumer *specificConsumer;


Yeah, there's a bit of Smalltalk-oidal transformative experience going on on my part here. Here's how I would push back: In this case, it would be a function called findSpecificConsumerIfAny and I could just read the name and know what's going on in the caller. Without the separate function, I need to read the body of the switch to see that it is setting the variable, and then also read further to see that the variable is never changed again. (Swift is way better:)

I realize that the uninitialized variable followed by conditional code that sets it is a common idiom in C(++), but my other beef is that the compiler does nothing to guarantee it is not modified below. When functions get longer, I always have to grep for the variable to ensure it doesn't get modified in the middle.

This particular case is not that bad, but I feel that there is something like "the death of a thousand cuts", where enough not-too-tough cases have a cumulative effect on the effort required to dive into an unfamiliar code base.

davidungar · 2018-03-01T18:31:06Z

lib/AST/DiagnosticConsumer.cpp

+  // behavior.
+  bool hadError = false;
+  for (auto &subConsumer : SubConsumers)
+    hadError |= subConsumer.second->finishProcessing();


davidungar · 2018-03-01T18:35:57Z

lib/FrontendTool/FrontendTool.cpp

-  // FIXME: Do we need one SerializedConsumer per primary? Owned by the
-  // SourceFile?
-  std::unique_ptr<DiagnosticConsumer> SerializedConsumer;
+  std::unique_ptr<DiagnosticConsumer> SerializedConsumerMultiplexer;


SerializedDiagnosticConsumerSorter? SerializedDiagnosticDistributor?? What's this thing doing? Directing diagnostics to an ultimate consumer?? DiagnosticPachinko? (No, that would be random.)

davidungar · 2018-03-01T18:37:06Z

lib/FrontendTool/FrontendTool.cpp

-        Invocation.getSerializedDiagnosticsPathForAtMostOnePrimary();
-    if (!SerializedDiagnosticsPath.empty()) {
-      SerializedConsumer.reset(
+    SmallVector<FileSpecificDiagnosticConsumer::ConsumerPair, 4>


Regarding the 4, how about a comment explaining why 4?

davidungar · 2018-03-01T18:47:41Z

lib/FrontendTool/FrontendTool.cpp

-        Invocation.getSerializedDiagnosticsPathForAtMostOnePrimary();
-    if (!SerializedDiagnosticsPath.empty()) {
-      SerializedConsumer.reset(
+    SmallVector<FileSpecificDiagnosticConsumer::ConsumerPair, 4>


Regarding factoring the vector-building: lines 1627 - 1653 are all computing one thing, the SerializedConsumerMultiplexer. But I had to read it carefully to see that that was the only thing it was doing, and that the multiplexer is not modified thereafter. Likewise for the 20-ish lines to compute the vector. It feels like there are functions being computed, but it could be easier to see that than what is apparent at a glance.

davidungar · 2018-03-01T18:48:37Z

lib/FrontendTool/FrontendTool.cpp

-      SerializedConsumer.reset(
+    SmallVector<FileSpecificDiagnosticConsumer::ConsumerPair, 4>
+      SerializedConsumers;
+    Invocation.getFrontendOptions().InputsAndOutputs


OK. Whatever the common practice is for our team is OK with me.

davidungar · 2018-03-01T18:52:45Z

lib/FrontendTool/FrontendTool.cpp

+      return false;
+    });
+
+    if (!SerializedConsumers.empty()) {


Is there a conceivable situation where the time difference would be noticeable? I had assumed that the extra time for a diagnostic (when there are no serialized consumers) would be unobservable, given the time required to generate the diagnostic, but maybe I'm wrong. It would be good for me to correct my intuition.

If the if-test is merely an optimization, and not needed for correctness, it would help to add a comment: "Save time when there are no SerializedConsumers" so that future maintainers know it is just an optimization.

If a top-level diagnostic is in a particular source range, the RangeSpecificDiagnosticConsumer will funnel it and any attached notes to a particular "sub-consumer" designated for that range (intended to be used with whole files). If it's not in a range designated for any sub-consumer, the diagnostic is passed to all registered sub-consumers. This is intended to be used for batch mode, so that diagnostics that are definitely associated with a particular file can be emitted in that file's .dia output, while diagnostics that may be associated with other files (such as those that come from Clang) will still get presented to the user.

It turns out that we need to have the diagnostic consumers set up before we've actually opened the input files, which makes sense because we might want to emit diagnostics about not being able to open an input file. Switch to using file names instead, and mapping those over to source ranges only once we actually need to handle a diagnostic with a valid source location.

It appears to work!

jrose-apple · 2018-03-02T21:25:55Z

Last update incorporating some parts of David's feedback and not other parts. ;-) Per his comment above I'm going to go with this version unless there are serious objections.

@swift-ci Please smoke test

graydon

Looks like a solid improvement to me.

jrose-apple · 2018-03-03T01:08:41Z

Bot got stuck, trying again.

@swift-ci Please smoke test Linux

jrose-apple requested review from graydon and davidungar March 1, 2018 02:31

jrose-apple mentioned this pull request Mar 1, 2018

Add a RangeSpecificDiagnosticConsumer #14844

Closed

davidungar reviewed Mar 1, 2018

View reviewed changes

jrose-apple force-pushed the mux branch from 3d345a8 to 416b439 Compare March 2, 2018 00:05

jrose-apple commented Mar 2, 2018

View reviewed changes

jrose-apple mentioned this pull request Mar 2, 2018

[DO NOT MERGE] Extra testing for FileSpecificDiagnosticConsumer #14927

Closed

davidungar approved these changes Mar 2, 2018

View reviewed changes

jrose-apple added 3 commits March 2, 2018 13:23

Wire up FileSpecificDiagnosticConsumer for serialized diagnostics

de74836

It appears to work!

jrose-apple force-pushed the mux branch from 416b439 to de74836 Compare March 2, 2018 21:23

graydon approved these changes Mar 3, 2018

View reviewed changes

graydon merged commit 0cd182d into swiftlang:master Mar 3, 2018

jrose-apple deleted the mux branch March 3, 2018 02:38

This was referenced May 11, 2018

[4.2] Fixes for batch mode diagnostics in Xcode #16529

Merged

[4.2 EARLY] Fixes for batch mode diagnostics in Xcode #16530

Merged

Wire up FileSpecificDiagnosticConsumer for serialized diagnostics #14900

Wire up FileSpecificDiagnosticConsumer for serialized diagnostics #14900

Uh oh!

Conversation

jrose-apple commented Mar 1, 2018

Uh oh!

jrose-apple commented Mar 1, 2018

Uh oh!

swift-ci commented Mar 1, 2018

Uh oh!

swift-ci commented Mar 1, 2018

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!

davidungar Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

davidungar Mar 1, 2018 •

edited

Loading