From be11d53bb25d4951a71568e92edfff26bd4f298c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 18 Aug 2020 13:24:09 +0200 Subject: [PATCH 1/3] Removed incorrect MethodImpl.NoInlining attribute Quick-fix PR to remove an unnecessary `[MethodImpl(MethodImplOptions.NoInlining)]` attribute from a throw helper method. That attribute caused the JIT not to see the throw in the body, resulting in the caller branch assuming the forward path being taken (the faulty one). This produced worse codegen, as the non-faulting path was always forced to have an extra jump ahead. --- .../Collections/ObservableGroupedCollectionExtensions.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs b/Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs index 69e17bbc085..b52d778f560 100644 --- a/Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs +++ b/Microsoft.Toolkit/Collections/ObservableGroupedCollectionExtensions.cs @@ -444,7 +444,6 @@ private static void RemoveItemAtWithLinq( /// /// Throws a new when a key is not found. /// - [MethodImpl(MethodImplOptions.NoInlining)] private static void ThrowArgumentExceptionForKeyNotFound() { throw new InvalidOperationException("The requested key was not present in the collection"); From f71cd1ae2aa1c34872057e6a35b7257eecc6ff08 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 18 Aug 2020 13:40:15 +0200 Subject: [PATCH 2/3] Improved codegen, moved throw code to external method Also added a more detailed exception message for users --- .../ReadOnlyObservableGroupedCollection.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs b/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs index 9ff14698586..661edda8ca2 100644 --- a/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs +++ b/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs @@ -52,8 +52,16 @@ private void OnSourceCollectionChanged(object sender, NotifyCollectionChangedEve // reporting the changes one by one. We consider only this case for now. if (e.OldItems?.Count > 1 || e.NewItems?.Count > 1) { - Debug.Fail("OldItems and NewItems should contain at most 1 item"); - throw new NotSupportedException(); + static void ThrowNotSupportedException() + { + throw new NotSupportedException( + "ReadOnlyObservableGroupedCollection doesn't support operations on multiple items at once.\n" + + "If this exception was thrown, it likely means support for batched item updates has been added to the " + + "underlying ObservableCollection type, and this implementation doesn't support that feature yet.\n" + + "Please consider opening an issue in https://github.com/windows-toolkit/WindowsCommunityToolkit to report this."); + } + + ThrowNotSupportedException(); } var newItem = e.NewItems?.Cast>()?.FirstOrDefault(); From e0f1cb3092a7e28bc8d69c6d8bb516ab3aefa41a Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Tue, 18 Aug 2020 15:22:16 -0700 Subject: [PATCH 3/3] Update Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs --- .../Collections/ReadOnlyObservableGroupedCollection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs b/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs index 661edda8ca2..42d012356f7 100644 --- a/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs +++ b/Microsoft.Toolkit/Collections/ReadOnlyObservableGroupedCollection.cs @@ -58,7 +58,7 @@ static void ThrowNotSupportedException() "ReadOnlyObservableGroupedCollection doesn't support operations on multiple items at once.\n" + "If this exception was thrown, it likely means support for batched item updates has been added to the " + "underlying ObservableCollection type, and this implementation doesn't support that feature yet.\n" + - "Please consider opening an issue in https://github.com/windows-toolkit/WindowsCommunityToolkit to report this."); + "Please consider opening an issue in https://aka.ms/windowstoolkit to report this."); } ThrowNotSupportedException();