From d7e4617ea13c4b0d9bb97871ab08dcc4c3675b80 Mon Sep 17 00:00:00 2001 From: Marek Safar Date: Wed, 11 Nov 2020 10:50:03 +0100 Subject: [PATCH] Revert "Revert "Use Dictionary for underlying cache of ResourceSet (#44104)" (#44482)" This reverts commit a5c0b09b8fb04bbb8bc826c69343ac16f5d83444. --- .../src/System/Resources/ResourceSet.cs | 107 +++++++----------- .../tests/ResourceSetTests.cs | 64 +++++++++++ 2 files changed, 107 insertions(+), 64 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs index 6ad05b6869293..6136c51f6e575 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs @@ -1,18 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -/*============================================================ -** -** -** -** -** -** Purpose: Culture-specific collection of resources. -** -** -===========================================================*/ - using System.Collections; +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.IO; @@ -29,15 +19,15 @@ namespace System.Resources public class ResourceSet : IDisposable, IEnumerable { protected IResourceReader Reader = null!; - internal Hashtable? Table; - private Hashtable? _caseInsensitiveTable; // For case-insensitive lookups. + private Dictionary? _table; + private Dictionary? _caseInsensitiveTable; // For case-insensitive lookups. protected ResourceSet() { // To not inconvenience people subclassing us, we should allocate a new // hashtable here just so that Table is set to something. - Table = new Hashtable(); + _table = new Dictionary(); } // For RuntimeResourceSet, ignore the Table parameter - it's a wasted @@ -96,7 +86,7 @@ protected virtual void Dispose(bool disposing) } Reader = null!; _caseInsensitiveTable = null; - Table = null; + _table = null; } public void Dispose() @@ -134,10 +124,8 @@ IEnumerator IEnumerable.GetEnumerator() private IDictionaryEnumerator GetEnumeratorHelper() { - Hashtable? copyOfTable = Table; // Avoid a race with Dispose - if (copyOfTable == null) - throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - return copyOfTable.GetEnumerator(); + // Avoid a race with Dispose + return _table?.GetEnumerator() ?? throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); } // Look up a string value for a resource given its name. @@ -145,48 +133,37 @@ private IDictionaryEnumerator GetEnumeratorHelper() public virtual string? GetString(string name) { object? obj = GetObjectInternal(name); - try - { - return (string?)obj; - } - catch (InvalidCastException) - { - throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } + if (obj is string s) + return s; + + if (obj is null) + return null; + + throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); } public virtual string? GetString(string name, bool ignoreCase) { - object? obj; - string? s; - // Case-sensitive lookup - obj = GetObjectInternal(name); - try - { - s = (string?)obj; - } - catch (InvalidCastException) - { + object? obj = GetObjectInternal(name); + if (obj is string s) + return s; + + if (obj is not null) throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } - // case-sensitive lookup succeeded - if (s != null || !ignoreCase) - { - return s; - } + if (!ignoreCase) + return null; // Try doing a case-insensitive lookup obj = GetCaseInsensitiveObjectInternal(name); - try - { - return (string?)obj; - } - catch (InvalidCastException) - { - throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } + if (obj is string si) + return si; + + if (obj is null) + return null; + + throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); } // Look up an object value for a resource given its name. @@ -208,13 +185,12 @@ private IDictionaryEnumerator GetEnumeratorHelper() protected virtual void ReadResources() { - Debug.Assert(Table != null); + Debug.Assert(_table != null); Debug.Assert(Reader != null); IDictionaryEnumerator en = Reader.GetEnumerator(); while (en.MoveNext()) { - object? value = en.Value; - Table.Add(en.Key, value); + _table.Add(en.Key, en.Value); } // While technically possible to close the Reader here, don't close it // to help with some WinRes lifetime issues. @@ -225,35 +201,38 @@ protected virtual void ReadResources() if (name == null) throw new ArgumentNullException(nameof(name)); - Hashtable? copyOfTable = Table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - return copyOfTable[name]; + copyOfTable.TryGetValue(name, out object? value); + return value; } private object? GetCaseInsensitiveObjectInternal(string name) { - Hashtable? copyOfTable = Table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - Hashtable? caseTable = _caseInsensitiveTable; // Avoid a race condition with Close + Dictionary? caseTable = _caseInsensitiveTable; // Avoid a race condition with Close if (caseTable == null) { - caseTable = new Hashtable(StringComparer.OrdinalIgnoreCase); - - IDictionaryEnumerator en = copyOfTable.GetEnumerator(); - while (en.MoveNext()) + caseTable = new Dictionary(copyOfTable.Count, StringComparer.OrdinalIgnoreCase); + foreach (var item in copyOfTable) { - caseTable.Add(en.Key, en.Value); + if (item.Key is not string s) + continue; + + caseTable.Add(s, item.Value); } _caseInsensitiveTable = caseTable; } - return caseTable[name]; + caseTable.TryGetValue(name, out object? value); + return value; } } } diff --git a/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs b/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs index 2975326fe4619..57837b922498b 100644 --- a/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs +++ b/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs @@ -5,6 +5,7 @@ using System.IO; using System.Reflection; using System.Globalization; +using System.Collections; using System.Collections.Generic; namespace System.Resources.Tests @@ -101,6 +102,69 @@ public void GetString() } } + public class ResourceSetTests_IResourceReader + { + class SimpleResourceReader : IResourceReader + { + Hashtable data; + public SimpleResourceReader() + { + data = new Hashtable(); + data.Add(1, "invalid"); + data.Add("String", "message"); + data.Add("Int32", 5); + } + + IEnumerator IEnumerable.GetEnumerator() + { + throw new NotSupportedException(); + } + + public IDictionaryEnumerator GetEnumerator() + { + return data.GetEnumerator(); + } + + public void Close() + { + } + + public void Dispose() + { + } + } + + [Fact] + public void Empty_Ctor() + { + Assert.Throws(() => new ResourceSet(null as IResourceReader)); + } + + [Fact] + public void GetObject() + { + var rs = new ResourceSet(new SimpleResourceReader()); + + Assert.Null(rs.GetObject("DoesNotExist")); + Assert.Null(rs.GetObject("1")); + + Assert.Equal(5, rs.GetObject("Int32")); + Assert.Equal(5, rs.GetObject("int32", true)); + } + + [Fact] + public void GetString() + { + var rs = new ResourceSet(new SimpleResourceReader()); + + Assert.Null(rs.GetString("DoesNotExist")); + Assert.Null(rs.GetString("1")); + + Assert.Equal("message", rs.GetString("String")); + Assert.Equal("message", rs.GetString("string", true)); + } + } + public class ResourceSetTests_StreamCtor : ResourceSetTests { public override ResourceSet GetSet(string base64Data)