Clean up outdated string comparison logic #19452

rbuckton · 2017-10-24T21:01:05Z

While looking at a different change I noticed that we had some incorrect logic in our string comparison functions due to code motion through multiple changes.

In general, the best mechanism to compare strings (in either a case-sensitive or a case-insensitive manner) is to leverage Intl.Collator. This allows us to perform string comparisons using the host culture without needing to perform string transformations for case-insensitive comparisons.

When Intl is not available, we first try to fall back to String.prototype.localeCompare before we finally fall back to an ordinal comparison using <.

In 58d69cd we found that localeCompare did not provide accurate results in Node v0.10 and introduced a fix to address this case.

In d7e33c9 we switched to using Intl.Collator for string comparisons in core.ts

In 2bb4abc we moved the localeCompare fix to core.ts, but in doing so it seems we ended up with an incorrect check. Rather than falling back to String.prototype.localeCompare when Intl is not available, we only perform the check when Intl is available. As such, we will never actually hit the case on Line 1060 as Node v0.10 does not support Intl.

This change cleans up our string comparison logic and fallback behavior such that:

Intl.Collator is used, if available, for case-insensitive string comparisons.
In the event Intl.Collator is unavailable, String.prototype.localeCompare and String.prototype.toLocaleUpperCase are used (except in the case of Node v0.10.0 where it provides incorrect results). For case-insensitive comparisons we use toLocaleUpperCase to properly support comparisons of Unicode characters such as ẞ (German uppercase sharp s) that do not properly round-trip to lowercase and the Turkish (dotted) i and (dotless) ı when the host locale specifies the Turkish alphabet.
In the worst case scenario we fall back to ordinal comparisons (< and >), as well as using String.prototype.toUpperCase for case-insensitive comparisons. toUpperCase still allows for case-insensitive comparisons of ẞ (German uppercase sharp s), but cannot properly handle the Turkish i.

This change does not introduce any new tests, as its minimum criteria is to avoid regressions in existing tests.

…als in loops

ghost · 2017-10-24T21:19:44Z

src/compiler/program.ts

@@ -1105,7 +1105,8 @@ namespace ts {
               return compareStrings(file.fileName, getDefaultLibraryFileName(), /*ignoreCase*/ !host.useCaseSensitiveFileNames()) === Comparison.EqualTo;
            }
            else {
-                return forEach(options.lib, libFileName => compareStrings(file.fileName, combinePaths(defaultLibraryPath, libFileName), /*ignoreCase*/ !host.useCaseSensitiveFileNames()) === Comparison.EqualTo);
+                const stringComparer = host.useCaseSensitiveFileNames() ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
+                return forEach(options.lib, libFileName => stringComparer(file.fileName, combinePaths(defaultLibraryPath, libFileName)) === Comparison.EqualTo);


Should this really depend on the locale the compiler is run in? We should avoid a situation where there are compiler errors in some countries but not others.

That's already how we do it today. In general we should use the sorting rules of the system locale, as that is most pleasing to the end user. In this case, differing locales could affect compile-time behavior. In .NET I would leverage the "invariant culture" for such things, but there is no such concept in Intl. We could consider using "en-US" as the "invariant culture", in which case we would want a compareStringsUI that uses the default culture for things like sorting diagnostic messages.

Note that "en-US" most likely won't correctly handle Turkish i on a case-insensitive file system. "en-US" treats i = I, whereas in a Turkish locale, i ≠ I but rather i = İ and ı = I.

This is not a sorting issue but an equality issue. Why not just use exact equality in the case-sensitive case and JS's builtin .toLowerCase() in the case-insensitive case? Or look at what the windows file system does, since I think that's the only one that matters for the case-insensitive case.

toLowerCase does not correctly handle ẞ (German uppercase sharp s), see https://en.wikipedia.org/wiki/Capital_%E1%BA%9E for reference.

Perhaps we should have an equalStrings function for equality cases. Its still generally better to leverage Intl than to perform a strict ordinal comparison as there exists the possibility that two unicode characters are still considered equal (such as cases where there exists both a single character form as well as a multi-character form that uses combining diacritic markers). If we added an equalStrings function for that case, then we would use an Intl.Collator whose usage is "search" rather than "sort".

HFS on darwin is also case-insensitive.

The most reliable way to compare two physical paths that point to actual files would be to perform an fs.statSync on both paths and compare the dev, rdev, and ino values. However, a stat is expensive.

I did some performance comparisons of CS and CI comparisons using Intl.Collator, localeCompare, and ordinal comparison. While ordinal wins handily in all cases (which is expected), Intl.Collator is oddly slower than localeCompare for CS (which is unexpected given there is no transformation).

While there may be some benefit to continuing to use Intl.Collator or localeCompare for case-sensitive comparisons, we don't do that today so I'll revert that part of this PR.

rbuckton · 2017-10-24T21:27:24Z

src/services/navigateTo.ts

@@ -176,11 +176,8 @@ namespace ts.NavigateTo {
    function compareNavigateToItems(i1: RawNavigateToItem, i2: RawNavigateToItem): number {
        // TODO(cyrusn): get the gamut of comparisons that VS already uses here.
        // Right now we just sort by kind first, and then by name of the item.
-        // We first sort case insensitively.  So "Aaa" will come before "bar".


compareStringsCaseSensitive already sorts this correctly. It is possible this was written to address the Node v0.10.0 issue with String.prototype.localeCompare, but it no longer seems necessary.

After reverting compareStringsCaseSensitive to just use an ordinal relational comparison, it seems that "aaa" is once again greater than "Aaa" (due to the fact the char code for "a" is 97 while the char code for "A" is 65).

…rting)

rbuckton · 2017-10-25T18:12:42Z

@Andy-MS, any other comments?

ghost · 2017-10-25T18:39:58Z

src/compiler/core.ts

@@ -656,13 +650,13 @@ namespace ts {
    }

    // TODO: fixme (N^2) - add optional comparer so collection can be sorted before deduplication.
-    export function deduplicate<T>(array: ReadonlyArray<T>, areEqual?: (a: T, b: T) => boolean): T[] {
+    export function deduplicate<T>(array: ReadonlyArray<T>, equalityComparer: (a: T, b: T) => boolean = equateValues): T[] {


Is the TODO fixed? We have a comparer and we ensure it always has a value.

No. I didn't add this TODO, but I believe it is talking about a Comparer<T> to sort the array, not the EqualityComparer<T> which just checks for equivalence. I'm not planning on making that change in this PR.

ghost · 2017-10-25T18:52:37Z

src/compiler/core.ts

+    /**
+     * Compare two values for their equality.
+     */
+    export function equateValues<T>(a: T | undefined, b: T | undefined) {


Nit: Accepting T | undefined for an arbitrary T is redundant because T could already be a union type.

ghost · 2017-10-25T18:53:40Z

src/compiler/core.ts

+    }
+
+    // Gets string comparers compatible with the current host
+    function createCaseInsensitiveStringCollator() {


Nit: Only called once immediately after its definition, just use an IIFE?

It looks cleaner without the IIFE.

ghost · 2017-10-25T18:55:47Z

src/compiler/core.ts

+     * If supported by the host, the default locale is used for comparisons. Otherwise, an ordinal
+     * comparison is used.
+     */
+    export function compareStringsCaseInsensitive(a: string | undefined, b: string | undefined) {


Mention in the name of the method that this is locale-sensitive.

I mentioned this to @mhegazy last night. We may just use en-US as an explicit "invariant" locale (per a recommendation from @bterlson).

ghost · 2017-10-25T18:57:40Z

src/compiler/core.ts

+     * If supported by the host, the default locale is used for comparisons. Otherwise, an ordinal
+     * comparison is used.
+     */
+    export function equateStringsCaseInsensitive(a: string | undefined, b: string | undefined) {


When does this have a different result from ===?

Its a case-insensitive comparison, so when comparing Aaa to aaa, etc.

ghost · 2017-10-25T18:59:59Z

src/compiler/core.ts

+    /**
+     * Performs a case-sensitive equality comparison between two strings.
+     */
+    export function equateStringsCaseSensitive(a: string | undefined, b: string | undefined) {


export const equateStringsCaseSensitive: (a: string | undefined, b: string | undefined) => boolean = equateValues;

It will be inlined, plus the const shows up as a variable and not a function in intellisense:

Not a big deal, but we have plenty of other cases where we write functions like this.

ghost · 2017-10-25T19:00:48Z

src/compiler/core.ts

+        return equateValues(a, b);
+    }
+
+    export function equateStrings(a: string | undefined, b: string | undefined, ignoreCase?: boolean) {


Make ignoreCase mandatory, else we should be using === directly.

ghost · 2017-10-25T19:02:09Z

src/compiler/core.ts

@@ -1904,8 +2003,9 @@ namespace ts {
        const aComponents = getNormalizedPathComponents(a, currentDirectory);
        const bComponents = getNormalizedPathComponents(b, currentDirectory);
        const sharedLength = Math.min(aComponents.length, bComponents.length);
+        const comparer = ignoreCase ? compareStringsCaseInsensitive : compareStringsCaseSensitive;


Would be easier to read if you just called compareStrings inside the loop -- then we wouldn't need a compareStringsCaseSensitive function and could just use === directly.

That's what this did before. However, that means we are evaluating the ignoreCase condition for each iteration of the loop. This saves a few cycles.

I would think that an indirect function call isn't any faster than an if?

I've actually removed compareStrings now. Comparisons need to be explicit and intentional:

compareValues now only compares numbers

compareStringsCaseSensitive compares strings using an ordinal case-sensitive comparison

compareStringsCaseSensitiveUI compares strings using a dictionary-ordered case-sensitive comparison using the current UI locale

compareStringsCaseInsensitive compares strings using an ordinal case-insensitive comparison

compareStringsCaseInsensitiveUI compares strings using a dictionary-ordered case-insensitive comparison using the current UI locale

ghost · 2017-10-25T19:03:07Z

src/compiler/parser.ts

@@ -6069,7 +6069,7 @@ namespace ts {
                    const checkJsDirectiveMatchResult = checkJsDirectiveRegEx.exec(comment);
                    if (checkJsDirectiveMatchResult) {
                        checkJsDirective = {
-                            enabled: compareStrings(checkJsDirectiveMatchResult[1], "@ts-check", /*ignoreCase*/ true) === Comparison.EqualTo,
+                            enabled: equateStrings(checkJsDirectiveMatchResult[1], "@ts-check", /*ignoreCase*/ true),


Can just call equateStringsCaseInsensitive directly.
But this case it might make more sense to just use ASCII capitalization (.toLowerCase()) instead of anything locale-sensitive.

ghost · 2017-10-26T14:07:21Z

src/compiler/core.ts

+    /**
+     * Compare two values for their equality.
+     */
+    export function equateValues<T>(a: T, b: T) {


Not used as an export

ghost · 2017-10-26T14:08:18Z

src/compiler/core.ts

+            && a.toUpperCase() === b.toUpperCase();
+    }
+
+    export function equateStringsCaseSensitive(a: string, b: string) {


Export not used

ghost · 2017-10-26T14:09:33Z

src/harness/harness.ts

@@ -1698,7 +1698,8 @@ namespace Harness {

        export function *iterateOutputs(outputFiles: Harness.Compiler.GeneratedFile[]): IterableIterator<[string, string]> {
            // Collect, test, and sort the fileNames
-            outputFiles.sort((a, b) => ts.compareStrings(cleanName(a.fileName), cleanName(b.fileName)));
+            const comparer = ts.getStringComparer(/*ignoreCase*/ false);


Just use compareStringsCaseInsensitive directly

ghost · 2017-10-26T14:09:43Z

src/harness/unittests/compileOnSave.ts

@@ -13,8 +13,9 @@ namespace ts.projectSystem {
    describe("CompileOnSave affected list", () => {
        function sendAffectedFileRequestAndCheckResult(session: server.Session, request: server.protocol.Request, expectedFileList: { projectFileName: string, files: FileOrFolder[] }[]) {
            const response = session.executeCommand(request).response as server.protocol.CompileOnSaveAffectedFileListSingleProject[];
-            const actualResult = response.sort((list1, list2) => compareStrings(list1.projectFileName, list2.projectFileName));
-            expectedFileList = expectedFileList.sort((list1, list2) => compareStrings(list1.projectFileName, list2.projectFileName));
+            const comparer = getStringComparer(/*ignoreCase*/ false);


Just use compareStringsCaseInsensitive directly

ghost · 2017-10-26T14:10:50Z

src/compiler/core.ts

+        return comparer(a, b);
+    }
+
+    export function compareStringsCaseSensitiveUI(a: string, b: string) {


This function is called CaseSensitive but /*caseInsensitive*/ true?

ghost · 2017-10-26T14:13:38Z

src/services/navigateTo.ts

-        return i1.matchKind - i2.matchKind ||
-            ts.compareStringsCaseInsensitive(i1.name, i2.name) ||
-            ts.compareStrings(i1.name, i2.name);
+        return compareValues(i1.matchKind, i2.matchKind)


This may run inside of a unit test, causing that test to fail for developers from other countries...

I've added a beforeEach/afterEach that explicitly sets the UI locale to "en-US" before running each test.

ghost · 2017-10-26T21:59:05Z

src/compiler/core.ts

+     * @param comparer An optional `Comparer` used to sort entries before comparison. If supplied,
+     * results are returned in the original order found in `array`.
+     */
+    export function deduplicate<T>(array: ReadonlyArray<T>, equalityComparer?: EqualityComparer<T>, comparer?: Comparer<T>): T[] {


You could make equalityComparer and comparer required. This causes in error in combineProjectOutput, which happens to have a comparer handy and have sorted the array already prior to deduplicating -- it should probably use toDeduplicatedSortedArray.

ghost · 2017-10-26T22:01:51Z

src/compiler/core.ts

@@ -837,13 +863,28 @@ namespace ts {
    }

    /**
+     * Creates an array of integers starting at `from` (inclusive) and ending at `to` (exclusive).
+     */
+    function sequence(from: number, to: number) {


Could use function indices(array: any[]): number[] { return array.map((_, i) => i); }

ghost · 2017-10-26T22:07:47Z

src/compiler/core.ts

+        stableSortIndices(array, indices, comparer);
+
+        const deduplicated: number[] = [];
+        loop: for (const sourceIndex of indices) {


I might be missing something, but it seems like for an array with no duplicates this will do O(n^2) work? We loop over indices which has the same length as the input, then we loop over deduplicated which will have on average length n/2, and with no duplicates the continue doesn't matter.

ghost · 2017-10-26T22:10:20Z

src/compiler/core.ts

+        }
+    })();
+
+    let uiCS: Comparer<string> | undefined;


Nit: use a longer name. Also, might want to put these in an object to ensure they are all set at once.
Also, might as well just set uiCS and uiCI eagerly at the call to setUILocale?

I'd rather they were not set at once or created eagerly, I'd prefer these to be created lazily as needed (since they may not be needed).

ghost · 2017-10-26T22:15:13Z

src/compiler/core.ts

@@ -1926,9 +2185,10 @@ namespace ts {
            return false;
        }

+        // File-system comparisons should use predictable ordering


This comment isn't going to make sense in this context unless you mention that this is contrasted with using locale-sensitive operations.

ghost · 2017-10-26T22:16:32Z

src/compiler/types.ts

+    }
+
+    /* @internal */
+    export type Selector<T, U> = (v: T) => U;


I'm not so hot on this, this is basically just a way to avoid writing an argument type?

ghost · 2017-10-26T22:16:57Z

src/harness/fourslash.ts

@@ -3129,6 +3129,7 @@ Actual: ${stringify(fullActual)}`);
 ${code}
 })`;
        try {
+


ghost · 2017-10-26T22:18:16Z

src/harness/runner.ts

@@ -207,6 +207,11 @@ function beginTests() {
        ts.Debug.enableDebugInfo();
    }

+    // run tests in en-US by default.
+    const savedUILocale = ts.getUILocale();


I think you need to set savedUILocale inside the call to beforeEach in case the locale was set sometime between the declaration of savedUILocale and the test running.

Highly unlikely, but I'll make the change.

ghost · 2017-10-26T22:19:48Z

src/compiler/core.ts

+    /**
+     * Compare two values for their order relative to each other.
+     */
+    export function compareValues(a: number, b: number) {


Rename to compareNumbers?

I'd rather leave the name as is for the time being. Too many engineers on the team are used to it having this name. I've added an additional comment indicating where to find the string comparison functions, to ease the transition.

mhegazy · 2017-10-30T21:49:23Z

src/compiler/checker.ts

        function binarySearchTypes(types: Type[], type: Type): number {
-            let low = 0;


we explicitly had two copies of this function to make sure this one is always optimized. we heavily rely on this function in the checker.

Interestingly enough, this PR seems to have improved check time. I'll revert this specific change and re-run the benchmarks to see if there is any difference.

The difference between the version in the PR and just reverting this single change is negligible (0% to 0.4% better after reverting the change). In most cases that's just within the standard error. I'd argue that manual inlining has only a minor overall impact and it might be worth the change to just have a single binary search algorithm.

rbuckton · 2017-10-30T22:38:54Z

This change seems to improve compile-time across the board: compareStrings.txt

Notably, check time is improved by 4-6%.

rbuckton · 2017-10-31T00:22:25Z

Unifying deduplication for diagnostics also seems to improve perf:
compareStrings.txt

rbuckton · 2017-10-31T00:28:46Z

@Andy-MS, @mhegazy any further comments?

ghost · 2017-10-31T14:36:11Z

src/compiler/core.ts

+
+    export function contains<T>(array: ReadonlyArray<T>, value: T, equalityComparer?: EqualityComparer<T>): boolean {
+        if (array) {
+            return equalityComparer


Was this really faster than just using equateValues as a default?

ghost · 2017-10-31T14:50:12Z

src/compiler/core.ts

+    /**
+     * Deduplicates an unsorted array.
+     * @param equalityComparer An optional `EqualityComparer` used to determine if two values are duplicates.
+     * @param comparer An optional `Comparer` used to sort entries before comparison. If supplied,


If a comparer isn't supplied then we should also keep the original order because we push in that order.

Results are always supplied in the original order. I'll update the comment.

ghost · 2017-10-31T14:52:17Z

src/compiler/core.ts

@@ -738,7 +802,7 @@ namespace ts {
     * are not present in `arrayA` but are present in `arrayB`. Assumes both arrays are sorted
     * based on the provided comparer.
     */
-    export function relativeComplement<T>(arrayA: T[] | undefined, arrayB: T[] | undefined, comparer: Comparer<T> = compareValues, offsetA = 0, offsetB = 0): T[] | undefined {
+    export function relativeComplement<T>(arrayA: T[] | undefined, arrayB: T[] | undefined, comparer: Comparer<T>, offsetA = 0, offsetB = 0): T[] | undefined {


Looks like the offsetA and offsetB parameters are never used.

ghost · 2017-10-31T15:00:10Z

src/compiler/core.ts

+                return a < b ? Comparison.LessThan : a > b ? Comparison.GreaterThan : Comparison.EqualTo;
+            }
+
+            function compareCaseSensitiveDictionaryOrder(a: string, b: string) {


This is not really case sensitive except in the case of a tie. Is there any case where we don't want ties broken? Could we not just always do this?
Actually, this code is probably never hit by tests due to being a fallback -- is this consistent with the behavior of the function it's a fallback for?

Yes, this preserves the ordering expectation initially expected by the compareNavigateToItems function in services/navigateTo.ts.

a) Since this is a fallback, shouldn't the others have the same behavior?
b) Maybe compareNavigateToItems should just use a case-insensitive comparison?

a) Both Intl.Collator and correct implementations of localeCompare already perform dictionary collation for case-sensitive comparisons.
b) That would result in possible cases of mixed capitalization in results like Aaa, aaa, AAa where we actually want it sorted as AAa, Aaa, aaa.

ghost · 2017-10-31T15:02:03Z

src/compiler/core.ts

        for (const basePath of patterns.basePaths) {
            visitDirectory(basePath, combinePaths(currentDirectory, basePath), depth);
        }

        return flatten<string>(results);

        function visitDirectory(path: string, absolutePath: string, depth: number | undefined) {
-            let { files, directories } = getFileSystemEntries(path);


I liked the deconstruction... note that files and directories are each only used once, so you could move the sort call inside the for loop header to avoid mutating locals.

ghost · 2017-10-31T15:04:37Z

src/services/textChanges.ts

@@ -581,7 +581,7 @@ namespace ts.textChanges {
            return applyFormatting(nonformattedText, sourceFile, initialIndentation, delta, this.rulesProvider);
        }

-        private static normalize(changes: Change[]): Change[] {
+        private static normalize(changes: Change[]) {


Did the return type change?

mhegazy · 2017-11-02T21:32:04Z

tests/cases/fourslash/incrementalParsingDynamicImport1.ts

 //// })
 //// /*1*/

-verify.numberOfErrorsInCurrentFile(1);
+verify.numberOfErrorsInCurrentFile(2);


so what is the extra error that showed up here?

I fixed an issue in 967a426 where we were losing some deferred global diagnostics.

When we would call sortAndDeduplicateDiagnostics we would sort the diagnostics in place: 967a426#diff-66cec844e251a1918deb897eabca206bL1856

After changing that to sort into a new array, I found that we had a few cases where we would modify the sorted array when adding deferred diagnostics. This was the fix: 967a426#diff-942ae3cd2a8bbd85ed86a60cd7c43307R2324

rbuckton · 2017-11-04T06:21:09Z

@mhegazy I ran an updated benchmark with TFS compiling clean and this still reduces compile time even when there are no errors to deduplicate.

rbuckton · 2017-11-04T06:26:54Z

Updated benchmark

Clean up outdated string comparison logic

71ab810

rbuckton requested review from sandersn, DanielRosenwasser and mhegazy October 24, 2017 21:01

Manually inline a few cases to reduce repeated 'ignoreCase' condition…

763d17e

…als in loops

ghost reviewed Oct 24, 2017

View reviewed changes

rbuckton commented Oct 24, 2017

View reviewed changes

rbuckton added 6 commits October 24, 2017 15:50

Revert use of Intl/localeCompare for CS comparisons

5f8b392

Added 'equateStrings' for string equality comparisons (rather than so…

dee77ce

…rting)

Clean up existing equality comparisons

7bc3b73

Minor name change

27d0b9f

Revert change in navigateTo

6d4e2a0

Consistent naming

4d7923a

rbuckton requested a review from a user October 25, 2017 18:12

ghost mentioned this pull request Oct 25, 2017

compareValues only works for string/string or number/number #19475

Closed

ghost reviewed Oct 25, 2017

View reviewed changes

rbuckton added 5 commits October 25, 2017 16:30

More control over which collator to use in each situation

130c407

Simplify comparers

c0ed26e

Cleanup and reordering

dfa1ffe

Remove unnecessary comment

3605e4e

Remove unnecessary comment

e08f8d2

ghost reviewed Oct 26, 2017

View reviewed changes

rbuckton added 3 commits October 26, 2017 11:56

Further simplification

2443777

Cleanup, merge #19475

bfba32b

Ensure explicit default locale before each test

d9775cd

ghost reviewed Oct 26, 2017

View reviewed changes

rbuckton added 3 commits October 26, 2017 17:51

Improve performance of deduplication of sorted arrays

3cb1537

PR Feedback

e6cdd63

Updated comments

fcb8296

rbuckton added 2 commits October 29, 2017 13:24

PR Feedback, cleanup

1f961cd

Added comment

c83eeaa

mhegazy suggested changes Oct 30, 2017

View reviewed changes

Unify deduplication, fix deferred global diagnostics

967a426

ghost approved these changes Oct 31, 2017

View reviewed changes

rbuckton added 3 commits October 31, 2017 10:49

PR feedback

1d0a9ee

Assert arrays passed to relativeComplement are sorted

88e56f3

Remove case-insensitive UI comparisons for now

542a060

ghost approved these changes Oct 31, 2017

View reviewed changes

mhegazy reviewed Nov 2, 2017

View reviewed changes

Merge branch 'master' into compareStrings

31c3d44

Fix new lint error

ed914a8

mhegazy approved these changes Nov 6, 2017

View reviewed changes

rbuckton merged commit 3f248ec into master Nov 7, 2017

rbuckton deleted the compareStrings branch November 7, 2017 18:20

microsoft locked and limited conversation to collaborators Jun 14, 2018

@@ @@ -3129,6 +3129,7 @@ Actual: ${stringify(fullActual)}`); @@
               ${code}
               })`;
                       try {

		function binarySearchTypes(types: Type[], type: Type): number {
		let low = 0;

Clean up outdated string comparison logic #19452

Clean up outdated string comparison logic #19452

Uh oh!

Conversation

rbuckton commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rbuckton Oct 24, 2017 • 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!

rbuckton Oct 24, 2017 • 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!

rbuckton Oct 24, 2017 • 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!

rbuckton Oct 24, 2017 • 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!

rbuckton commented Oct 25, 2017

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

rbuckton commented Oct 24, 2017 •

edited

Loading

rbuckton Oct 24, 2017 •

edited

Loading

rbuckton Oct 24, 2017 •

edited

Loading

rbuckton Oct 24, 2017 •

edited

Loading

rbuckton Oct 24, 2017 •

edited

Loading

ghost Oct 26, 2017 •

edited by ghost

Loading

ghost Oct 26, 2017 •

edited by ghost

Loading